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

Adds support for using hifitime in WASM targets #262

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

thomasantony
Copy link
Contributor

std::time::SystemClock::now() does not work in the browser. The instant provides an alternative that works in the browser and falls back to std::time::SystemClock on non-WASM targets. I have added a new web feature flag that enables the extra features from the instant crate.

This is required for running the ANISE gui in the browser.

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b731275) 80.67% compared to head (85af13f) 80.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   80.67%   80.74%   +0.06%     
==========================================
  Files          15       16       +1     
  Lines        3746     3754       +8     
==========================================
+ Hits         3022     3031       +9     
+ Misses        724      723       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChristopherRabotin
Copy link
Member

Since the instant crate has not been updated since October 2018, uses the 2018 edition of Rust, and since SystemTime is only referenced twice I have an itch to just snatch the small amount of relevant code from that crate (BSD-3, so no license issue there), and skip using the crate all together.

I'll have a look at doing this over the weekend.

@thomasantony
Copy link
Contributor Author

@ChristopherRabotin I have moved over what I think is the minimal amount of code to get things going. The instant crate had a bunch of compile-time options that seemed to be about using the different web-runtimes. I copied over just the one for wasm-bindgen as that seems to be the most widely used one at this point (and that is what I compiled ANISE with).

The new dependencies are restricted to wasm targets.

Copy link
Member

@ChristopherRabotin ChristopherRabotin left a comment

Choose a reason for hiding this comment

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

Great work, this is exactly what I was asking for. I just have a few comments to use the Unit time in hifitime for the unit conversion from seconds to a Duration structure, and that should be good to merge.

pub(crate) fn now() -> f64 {
std::time::SystemTime::now().duration_since(std::time::SystemTime::UNIX_EPOCH)
.expect("System clock was before 1970.")
.as_secs_f64() * 1000.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.as_secs_f64() * 1000.0
.as_secs_f64() * Unit::Second

By using the Unit enum, the now() function would directly return a Duration, so the function could be directly duration_since_unix_epoch() I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I have refactored system_time:: duration_since_unix_epoch() to directly return hifitime::Duration

@@ -0,0 +1,27 @@
use std::time::Duration;
Copy link
Member

Choose a reason for hiding this comment

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

Should this import be moved into the now() function on line 16?

Copy link
Contributor Author

@thomasantony thomasantony Dec 10, 2023

Choose a reason for hiding this comment

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

It is also used in the return type for duration_since_unix_epoch

No longer relevant as I am now using hifitime::Duration

Ok(std_duration) => Ok(Self::from_unix_seconds(std_duration.as_secs_f64())),
Err(_) => Err(Errors::SystemTimeError),
}
let duration = crate::system_time::duration_since_unix_epoch()?;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this needs to be "from unix duration" now, otherwise it assumes that the reference epoch is J1900 (the TAI ref epoch), but unix seconds are wrt 01 January 1970

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 could not find a from_unix_duration(). Should I add it?

@ChristopherRabotin
Copy link
Member

ChristopherRabotin commented Dec 10, 2023 via email

@thomasantony
Copy link
Contributor Author

Ah, right, seems like it's only on the version 4 development branch. Just call the from unix seconds : https://docs.rs/hifitime/latest/hifitime/struct.Epoch.html#method.from_unix_seconds

On Sat, Dec 9, 2023, 23:19 Thomas Antony @.> wrote: @.* commented on this pull request. ------------------------------ In src/epoch.rs <#262 (comment)>: > pub fn now() -> Result<Self, Errors> { - match SystemTime::now().duration_since(SystemTime::UNIX_EPOCH) { - Ok(std_duration) => Ok(Self::from_unix_seconds(std_duration.as_secs_f64())), - Err(_) => Err(Errors::SystemTimeError), - } + let duration = crate::system_time::duration_since_unix_epoch()?; I could not find a from_unix_duration(). Should I add it? — Reply to this email directly, view it on GitHub <#262 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABEZV2AS6RSUO57QSFVADJDYIVH6DAVCNFSM6AAAAABAIYSNUWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZTHE3TEMRSGI . You are receiving this because you were mentioned.Message ID: @.***>

I created a from_unix_duration(). See my latest commit.

@ChristopherRabotin
Copy link
Member

Looks good, thank you

@ChristopherRabotin ChristopherRabotin merged commit a716712 into nyx-space:master Dec 11, 2023
31 checks passed
@thomasantony thomasantony deleted the add-web-support branch December 11, 2023 00:30
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.

2 participants