script: Start replacing time with std::time and chrono
#30639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrobinson
requested changes
Oct 29, 2023
components/script/script_runtime.rs
Outdated
Comment on lines
771
to
773
| let dur = SystemTime::now() | ||
| .duration_since(start.get().unwrap()) | ||
| .unwrap(); |
There was a problem hiding this comment.
I think we should try to not panic here, so please provide defaults if we cannot unwrap successfully.
There was a problem hiding this comment.
Changed the last unwrap to unwrap_or_default, for lack of a smarter solution.
components/script/script_runtime.rs
Outdated
Comment on lines
778
to
780
| let dur = SystemTime::now() | ||
| .duration_since(start.get().unwrap()) | ||
| .unwrap(); |
There was a problem hiding this comment.
components/script/script_thread.rs
Outdated
| @@ -258,7 +261,7 @@ impl InProgressLoad { | |||
| is_visible: true, | |||
| url: url, | |||
| origin: origin, | |||
| navigation_start: (current_time.sec * 1000 + current_time.nsec as i64 / 1000000) as u64, | |||
| navigation_start: navigation_start, | |||
There was a problem hiding this comment.
Suggested change
| navigation_start: navigation_start, | |
| navigation_start, |
There was a problem hiding this comment.
I contemplated doing this but it seems this would be better dealt with in a big fmt change.
There was a problem hiding this comment.
We could enforce it with use_field_init_shorthand.
|
I'm nearly done; only have EDIT: Immediately after commenting I got an answer on Zulip, which allowed me to get a solution that compiles; however, I will put in a bit more work to potentially get a more robust solution that doesn't involve EDIT: Seems like the |
augustebaum
force-pushed
the
time_script
branch
3 times, most recently
from
8a2d446
to
3da3176
Compare
augustebaum
force-pushed
the
time_script
branch
from
3da3176
to
59979aa
Compare
augustebaum
force-pushed
the
time_script
branch
from
59979aa
to
669a209
Compare
mrobinson
approved these changes
Oct 30, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Looks like this is a flake in the float tests...frustrating and they should probably be switched away from a fuzzing approach to a traditional test framework. I'm resending this to the MQ. |
|
@mrobinson Thanks for the review! Please consider resolving the above conversations as I'm interested in your opinion on these. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
It seems that some animation and performance tests are failing, likely because the new code is returning different results that the old code. One way to debug this is to compare the two times (via the old APIs and the new ones) via print statements to see where they are different. Failures: |
mrobinson
requested changes
Nov 3, 2023
augustebaum
force-pushed
the
time_script
branch
from
669a209
to
f227071
Compare
|
🔨 Triggering try run (#6747135167) with platforms=linux,macos,windows and layout=all |
|
Test results for linux-wpt-layout-2013 from try job (#6747135167): Flaky unexpected result (20)
Stable unexpected results that are known to be intermittent (21)
Stable unexpected results (7)
|
|
Test results for linux-wpt-layout-2020 from try job (#6747135167): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (4)
|
|
Some of these test failures might be linked to the fact that |
|
Where possible use https://doc.rust-lang.org/std/time/struct.Instant.html |
|
The new set of results looks a lot better! I think this is getting quite close. I would also rebase this as at least one of these failures should be fixed in the latest from the main branch. |
|
I'm reworking the changes and there is something I find strange. The servo/components/script/dom/globalscope.rs Line 207 in f227071 The first time they're used it's to put in a timestamp: servo/components/script/dom/globalscope.rs Line 2272 in f227071 whereas the second time it's to put in the elapsed time since the timestamp: servo/components/script/dom/globalscope.rs Line 2284 in f227071 Ideally |
augustebaum
force-pushed
the
time_script
branch
from
f227071
to
d4ed3e0
Compare
|
Test results for linux-wpt-layout-2020 from try job (#7531522298): Flaky unexpected result (10)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (3)
|
|
|
|
Looks like there is at least one more issue to address. |
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
….rs` Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Signed-off-by: Auguste Baum <auguste.apple@gmail.com>
Use Instant a bit more and stop using chrono. Do not transition `navigation_start_precise` to Instant yet as we need to coordinate this across all crates.
mrobinson
force-pushed
the
time_script
branch
from
e0742c1
to
6b95401
Compare
|
🔨 Triggering try run (#7540553241) with platforms=linux and layout=undefined |
|
Test results for linux-wpt-layout-2020 from try job (#7540553241): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (12)
|
|
✨ Try run (#7540553241) succeeded. |
mrobinson
changed the title
time with std::time in components/scripttime with std::time and chrono
|
I had to make some changes to get this working namely, fixing some precision issues in the animation timeline as well as not making the changes for |
mrobinson
approved these changes
Jan 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change is intended to be reviewed commit-by-commit.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorstime(precise_time_ns) withstd::time,chrono, or the newtimecrate #30150