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

feat(ecmascript): implement Date constructor and prototype methods #586

Merged
merged 22 commits into from
Mar 11, 2025

Conversation

kermanx
Copy link
Contributor

@kermanx kermanx commented Mar 8, 2025

This PR implements most of the Date constructor and prototype methods. Closes #139.

Currently only UTC timezone is supported. Some date string parsers have not been implemented.

Many of the implementations are ported from https://github.com/boa-dev/boa/blob/main/core/engine/src/builtins/date/utils.rs and https://github.com/boa-dev/boa/blob/main/core/engine/src/builtins/date/mod.rs

btw, how can I update matrix.json without running all the test262 on my local machine?

@kermanx kermanx changed the title feat(ecmascript): implement Date prototype methods feat(ecmascript): implement Date constructor and prototype methods Mar 9, 2025
@kermanx kermanx marked this pull request as ready for review March 9, 2025 07:43
@aapoalas
Copy link
Member

aapoalas commented Mar 9, 2025

Metrics / expectations can be updated with these instructions: https://github.com/trynova/nova/blob/main/CONTRIBUTING.md#tests-in-prs

Basically, this command:

cargo build --profile release && cargo run --bin test262 --profile release -- -u

Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Holy moly, amazing work! Thank you and massive props for taking on this large body of work!

I had a few issues here and there, and nits to pick. The most problematic issue is regarding the GC safety of some of the calls; I unfortunately haven't quite yet finished a comprehensive guide to working with the GC but a basic one can be found here and I'm writing a (hopefully) more comprehensive one in a blog format over here.

Beyond that, I wonder if we couldn't avoid using f64 quite so much since the millisecond numbers are always rounded down to integers and are always kept in the safe integer range. I would assume that would simplify and perhaps speed up a lot of the code working around dates.

But all in all, great work, you really knocked it off the park right off the bat with this one!

@kermanx
Copy link
Contributor Author

kermanx commented Mar 9, 2025

The most problematic issue is regarding the GC safety of some of the calls; I unfortunately haven't quite yet finished a comprehensive guide to working with the GC but a basic one can be found here and I'm writing a (hopefully) more comprehensive one in a blog format over here.

I read the blog posts and GARBAGE_COLLECTOR.md before PRing, but I didn't fully understand it. After reading the new draft of the blog post, I think I now have a better understanding of the idea.

@kermanx
Copy link
Contributor Author

kermanx commented Mar 9, 2025

Metrics / expectations can be updated with these instructions: main/CONTRIBUTING.md#tests-in-prs

It seems that cargo run --bin test262 --profile release -- -u runs for ~30 minutes on my machine and still does not finish. Is that normal? 🥹

@aapoalas
Copy link
Member

aapoalas commented Mar 9, 2025

Metrics / expectations can be updated with these instructions: main/CONTRIBUTING.md#tests-in-prs

It seems that cargo run --bin test262 --profile release -- -u runs for ~30 minutes on my machine and still does not finish. Is that normal? 🥹

No, that does not sound normal at all. On my machine running the tests is maybe two minutes on release mode, perhaps 5-7 minutes on debug.

What's your machine like?

@aapoalas
Copy link
Member

aapoalas commented Mar 9, 2025

Metrics / expectations can be updated with these instructions: main/CONTRIBUTING.md#tests-in-prs

It seems that cargo run --bin test262 --profile release -- -u runs for ~30 minutes on my machine and still does not finish. Is that normal? 🥹

Here are the exact commands I run locally:

cargo build && cargo build --profile release
cargo run -r --bin test262 -- -u

@kermanx
Copy link
Contributor Author

kermanx commented Mar 10, 2025

Here are the exact commands I run locally:

cargo build && cargo build --profile release
cargo run -r --bin test262 -- -u

I just ran the exact same commands and it takes 40~50 minutes to finish. Some information of my machine:

OS: Windows 11
CPU: 13th Gen Intel(R) Core(TM) i7-1360P   2.20 GHz
RAM: 32.0 GB

I guess the difference between Linux and Windows may cause the problem.

@r58Playz
Copy link
Contributor

I have the exact same CPU model and test262 runs in 16 seconds on Arch Linux:

________________________________________________________
Executed in   16.04 secs    fish           external
   usr time   92.10 secs    0.00 micros   92.10 secs
   sys time  105.97 secs  800.00 micros  105.97 secs

Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Some nits to pick still, and let's add unit tests for the f64 -> IntegerOrInfinity to test that it doesn't overflow with big f64 inputs.

I'm... not totally thrilled with f64 still remaining in the calculation. It seems to me like a DateValue kind of wrapper around i64 (or just using DateValue directly, though it's a bit wrong admittedly since it's supposed to represent milliseconds) for the functions would be a reasonable choice that would avoid the pain of "how to deal with NaN i64s?" while still allowing computation to proceed mostly without pain on the i64 paths.

But, this fine as well. Let's just get the few minor edges filed down and this is good to go! Great stuff, absolutely stellar work! Thank you <3

aapoalas
aapoalas previously approved these changes Mar 11, 2025
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Sounds good to me!

@aapoalas aapoalas merged commit 08ac2ab into trynova:main Mar 11, 2025
2 checks passed
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.

Date.prototype builtin functions
3 participants