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

Uses instant::Instant in place of std::time::Instant. #231

Merged

Conversation

azriel91
Copy link
Contributor

Heya, I was trying out governor (rate limiter) on WASM, which required parking_lot to use a wasm compatible Instant type, and I came up with this patch.

The underlying instant::Instant type is std::time::Instant when compiled without enabling the "stdweb" or "wasm-bindgen" features.

If you'd like to take it I'll be happy; I'm still trying out different rate-limiting implementations, so am not sure if I'll be using this (but if it's merged, then it's more likely that I will).

core/Cargo.toml Show resolved Hide resolved
@@ -51,7 +52,7 @@ cfg_if! {
// See https://github.com/rust-lang/rust/blob/master/src/libstd/sys/wasm/time.rs
type InstantType = DummyInstant;
} else {
// Otherwise use `std::time::Instant`
// Otherwise use `instant::Instant`
type InstantType = Instant;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this hack still necessary now that we have support for Instant on WASM? I guess the default instant type is still used if neither the stdweb or wasm-bindgen features are enabled...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but shall check (give me an hour)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works without the hack ✌️

GIF of a single threaded WASM countdown (counts down quickly, but only refreshes at 10 FPS):

wasm_countdown

Copy link
Owner

Choose a reason for hiding this comment

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

After thinking about it for a bit, I think it's better to keep the old DummyInstant hack for wasm. The code using it relies on the fact that calling Instant::now is reasonably fast, which may not be the case on wasm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, shall take take commit out

@azriel91
Copy link
Contributor Author

Build failure on x86_64-linux-android:

error: linking with `cc` failed: exit code: 1
# ..
  = note: /usr/bin/ld: cannot find -llog
          collect2: error: ld returned 1 exit status

Not sure if it's related to this change -- instant doesn't introduce any system dependencies

@Amanieu
Copy link
Owner

Amanieu commented May 15, 2020

I think it's unrelated, maybe there is some issue with Android CI?

cc @XAMPPRocky

@azriel91 azriel91 force-pushed the feature/use-wasm-compatible-instant branch from 26149d0 to 37ceeb3 Compare May 15, 2020 22:47
@Amanieu Amanieu merged commit 715d086 into Amanieu:master May 15, 2020
@azriel91 azriel91 deleted the feature/use-wasm-compatible-instant branch May 15, 2020 23:44
@XAMPPRocky
Copy link
Contributor

It looks like a regression in the nightly compiler to me. If it happens consistently I would report it to the main rust repo.

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.

3 participants