Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add web features to the parking_lot dependency #225

Closed
wants to merge 2 commits into from

Conversation

zetanumbers
Copy link
Contributor

Resolves #218.

The parking_lot for wasm32 on stable Rust will panic on attempt of parking thread. This makes it possible to rely on stable version of parking_lot either with or without the atomics wasm32 target feature present. Therefore it is a step towards the stable toolchain (#221). Then the last step would be to disable (maybe conditionally) the named-profiles cargo feature.

While similar PR #219 conflicts with #223, this PR won't.

Run local-wasm-pack in release mode *for speed*.
Copy link
Owner

@anp anp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the "proper" way to do this in cargo is for each of these crates to have their own wasm-bindgen features, and then to depend on those features only when building for wasm32-unknown-unknown. That's going to be really painful though, so I'm thinking that we should probably switch from parking_lot's locks to std::sync's locks and avoid the issue altogether.

What do you think? I'm not sure parking_lot gets us much except for slightly more readable code.

.cargo/config.toml Outdated Show resolved Hide resolved
@zetanumbers
Copy link
Contributor Author

I think the "proper" way to do this in cargo is for each of these crates to have their own wasm-bindgen features, and then to depend on those features only when building for wasm32-unknown-unknown. That's going to be really painful though...

Well this feature wasm-bindgen is forwarded to the instant crate. They specify that using these features on native platforms does nothing (means no conditional compilation is required), but not using any of them on wasm32 without wasi will cause linking of some functions from js environment (which was in the name of related issue btw). And I think wasm-bindgen is preferable to stdweb because of using the same deps, but i could be wrong.

...I'm thinking that we should probably switch from parking_lot's locks to std::sync's locks and avoid the issue altogether.

parking_lot has RwLock fair for writers policy, but std::sync's one is unspecified. This is going to matter with #223, bc with std::sync it would not guarantee (for example wasm32 target) that on Var::enqueue_commit could starve the Runtime::run_once at the start. While this is maybe not plausible, it would not be a guarantee. (Note that i should look into that in my PR.)

Also i don't see how conditional compilation in rust is too complicated (please don't listen to me, I had C++ template meta-programing in my homework)

@anp anp mentioned this pull request Dec 29, 2020
@anp
Copy link
Owner

anp commented Dec 29, 2020

Thank you again for this! It does locally resolve the chromedriver issue that is happening in CI, so I've cherry-picked these changes onto #226. Hopefully that will get CI unblocked.

Replying:

Well this feature wasm-bindgen is forwarded to the instant crate.

Exactly -- since dyn-cache, moxie, and topo don't know whether they're being run on the web or not, forwarding the feature seems right. While the feature is a no-op on non-web targets right now, that could change.

Don't worry about this, I'll take care of it in commits atop yours in #226. EDIT: done

And I think wasm-bindgen is preferable to stdweb because of using the same deps, but i could be wrong.

Yep, I don't think stdweb would even work here.

with std::sync it would not guarantee (for example wasm32 target) that on Var::enqueue_commit could starve the Runtime::run_once at the start

I'm not sure starvation is actually what we want, but it's straightforward enough to pull this fix in and revisit later!

@anp anp mentioned this pull request Dec 29, 2020
@anp
Copy link
Owner

anp commented Dec 29, 2020

Thank you again for finding this! I've landed #226 which includes these changes.

@anp anp closed this Dec 29, 2020
@zetanumbers zetanumbers deleted the parking_lot_fix branch December 29, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error resolving module specifier “env”
2 participants