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 DataView constructor #447

Merged

Conversation

eliassjogreen
Copy link
Member

@eliassjogreen eliassjogreen commented Oct 17, 2024

This PR implements the DataView constructor and it's getters. It should be rather easy to implement all of the get* and set* methods in a future PR and then DataView is basically fully implemented.

Partially completes #152

Copy link
Collaborator

@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.

First off, wow! You bit off a big piece and chewed it down like it wasn't even a problem!

The code seems to be perfectly correct already, and in that sense would be mergeable. However, there were some minor points about integer to double conversions, and Agent mutability things. Those should be fixed.

Then there's a larger thing about squishing down the size of a DataView. We can get it down to just 4 bytes (8 bytes with backing object) for the new DataView(ab) case, or perhaps 12 bytes (16 with backing object) if we want to avoid a performance cliff for new DataView(ab, 1, 2) kind of cases.

If you want to try that out, I'd be really happy to see that and I'll of course help if you have any questions. If you think it's too much effort or you don't have time, let me know and we'll merge this with the usize usage. (Though the Option<usize> do need to be fixed, that's a bit too much memory usage on a single bit for me to handle.)

But really good code, really cleanly done, and great job!

@eliassjogreen
Copy link
Member Author

Third option is to store a u32 here instead, and again use eg. u32::MAX or the top bit as a sentinel that points to the data_view_byte_lengths HashMap, and some other sentinel value (eg. u32::MAX - 1) serving as AUTO. That deoptimises the AUTO case somewhat but removes a performance cliff from small non-AUTO values, which is basically all DataViews. (Only very rarely would one find a DataView accessing into a 4 GB or more of data.)

I pushed some fixes, and opted for your third option which seemed the easiest. The only thing left is GC for the data_view_byte_lengths and data_view_byte_offsets hash maps which I might need some pointers for implementing (or if it's fine to leave it as a todo!)

Copy link
Collaborator

@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 minor cleanups needed.

The heap marking/sweeping stuff we need to do is to touch the agent.heap.data_view_byte_lengths and ..._offsets so that if we eg. originally have something like:

data_view_byte_lengths: HashMap {
  DataView(13) => X,
  DataView(15) => Y,
}

and then we do a GC where we notice that DataView(13) is no longer accessible and neither is DataView(14), then we need to sweep the HashMap so that the end result is

data_view_byte_lengths: HashMap {
  DataView(13) => Y,
}

@aapoalas
Copy link
Collaborator

Regarding the sweep stuff I mentioned:

and then we do a GC where we notice that DataView(13) is no longer accessible and neither is DataView(14), then we need to sweep the HashMap so that the end result is

This should happen in the fn sweep in heap_gc.rs. There should be a part there for sweeping data_views; that place should also then sweep the HashMaps to shift indexes down and remove any dropped indexes.

@eliassjogreen
Copy link
Member Author

eliassjogreen commented Oct 20, 2024

Took some time to understand and get acquainted with everything GC-related but I think I got it now. It seems to GC ok from my tests, but could definitely be optimized and improved. For a start it always deletes the moved values from the hashmap instead of only the ones which are not gonna get overwritten (e.g. the trailing ones).

Copy link
Collaborator

@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.

I'll finish the review at a later date: This is probably all good now but I left some notes and/or suggestions.

nova_vm/src/heap/heap_bits.rs Outdated Show resolved Hide resolved
nova_vm/src/heap/heap_bits.rs Show resolved Hide resolved
nova_vm/src/heap/heap_bits.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

LGTM! Thank you again and great job, you've now reached the lowest point of the engine in the GC :)

@aapoalas aapoalas merged commit 99f018b into trynova:main Oct 28, 2024
1 check 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.

2 participants