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

Introduce ordered slot map to eventually replace EmbeddedSlotMap #1821

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gbrail
Copy link
Collaborator

@gbrail gbrail commented Feb 8, 2025

This is a new SlotMap implementation that can replace EmbeddedSlotMap. It works just like the embedded slot map but for two things:

  • Slots are kept in an array rather than in a linked list
  • Deleted slots are replaced in the array with a sentinel
  • In the unlikely case that there are more than 10 deletes in the lifetime of the slot map, promote this map back to a HashSlotMap

This should not have a significant impact on performance yet. However it has two advantages for the future:

  1. By making the object graph less complicated, it might make GC perform better. @aardvark179 I'd be really interested to see if this change improves anything in your environment
  2. This opens up the possibility of a future optimization in compiled mode, because when we have the same property of the same object accessed at a given call site, we can replace the entire hash table operation with a direct array lookup
  3. In theory, if we implemented a "property map" like V8 has we could possibly expand that optimization even more

@aardvark179
Copy link
Contributor

I'll take a look. We should also definitely talk about future directions for optimising slot access.

In terms of recovering/improving benchmark performance the best improvement I've got has been from refactoring away from IdScriptable. I've got a prototype of this for NativeObject which needs one little fix around prototypes of security methods and then I'll make a draft PR for it, and seems to improve a bunch of compile benchmarks by about 10%.

@aardvark179
Copy link
Contributor

aardvark179 commented Feb 11, 2025

The motivation is good, but optimising an implementation like this, even in compiled mode, will be fraught as you have to ensure the slot is not removed and then added again (thus changing the property order), but this might be a step along the road to getting something that can be optimised more easily.

Looking at a few heap dumps and the code I think the important thing for this change to pay off will be to try and make it come out approximately neutral in terms of memory footprint as well making life easier for the GC. To achieve that I think it is worth making the position field a short so it can be packed in with the attributes,, and working to get rid of EmbeddedSlotMap and hence the orderedNext field. Those changes would shrink the Slot size down to 32 bytes on a 65bit VM with compressed oops, which should more than balance out the extra array.

I've prototyped a thread safe version, which found one bug in the use of dirtySize with that in place it's pretty trivial to remove EmbeddedSlotMap and its subclasses, and the orderedNext slot (pushed to https://github.com/aardvark179/rhino/tree/ordered-slot-map). I've run a couple of benchmark cases and don't see any regressions so far, so it looks pretty good to me.

# Conflicts:
#	rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java
@gbrail
Copy link
Collaborator Author

gbrail commented Feb 19, 2025

I can be slow on the uptake sometimes... I was going to change this class so that when an object is deleted, it keeps its order in the ordered slot map, but TBH that won't work without changing the iteration logic, because in JS if you delete a property and re-create one with the same name the iteration order changes. But I can work on an optimization that will only be active for objects that never saw a deletion, and that will cover the 90% that we need for good performance...

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