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

Single entry slot map #1784

Merged

Conversation

aardvark179
Copy link
Contributor

@aardvark179 aardvark179 commented Dec 30, 2024

Single entry slot map

This is a stacked PR on top of

This introduces a single-entry slot map to take the common case of a single property from:

graph LR
Object["ScriptableObject"]
Map["SlotMap"]
Array["Slot[]"]
Slot1["Slot"]
Object1["ScriptableObject"]
Map1["SlotMap"]
Array1["Slot[]"]
Slot2["Slot"]
Object --> Map --> Array
Array --> Slot1
Object1 --> Map1 --> Array1
Array1 --> Slot2
Loading

to:

graph LR
Object["ScriptableObject"]
Map["SingleEntrySlotMap"]
Slot1["Slot"]
Object1["ScriptableObject"]
Map1["SingleEntrySlotMap"]
Slot2["Slot"]
Object --> Map --> Slot1
Object1 --> Map1 --> Slot2
Loading

This saves 48 bytes on any object with a single slot.

@aardvark179 aardvark179 marked this pull request as ready for review December 30, 2024 16:31
@aardvark179 aardvark179 force-pushed the aardvark179-add-single-entry-slot-map branch from 477fa19 to 088d6ab Compare January 9, 2025 20:13
@aardvark179
Copy link
Contributor Author

Rebased after merge of #1782.

@aardvark179 aardvark179 force-pushed the aardvark179-add-single-entry-slot-map branch 2 times, most recently from fb1e844 to 1677a85 Compare January 17, 2025 13:33
Refactor `SingleSlotMap` to be immutable.
Refactor test to match `SlotMapTest` to match `SingleEntrySlotMap`.
@aardvark179 aardvark179 force-pushed the aardvark179-add-single-entry-slot-map branch from 1677a85 to 4f22b78 Compare January 23, 2025 14:26
@Override
public <S extends Slot> S compute(
SlotMapOwner owner, Object key, int index, SlotComputer<S> c) {
var newMap = new EmbeddedSlotMap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory, "compute" is also used to replace an existing slot, rather than add a new one. I don't know how much that happens in actual situations, but there are plenty of places in the codebase where it happens. With this implementation, in that case we'll upgrade to a "real" EmbeddedSlotMap, but then it still may have a size of one.

Would it complicate things a lot to support that use case? If not, should we try it? I would also be open to the idea that this would be a premature optimization.

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 chose to always promote there because I figured that a map which is mutated is likely to continue to be mutated, and it made it easier to think about the multithreaded cases.

Removing a slot also causes the map to be promoted as that maintains a strict type lattice of slot map types (we never demote a map). I tried to support that sort of thing initially but it made the code much more complex and was rarely triggered in my testing so I went for the simpler approach seen here.

Ultimately I think we should separate values and slot descriptors, and make the latter immutable. That would open up some good optimization opportunities in both the interpreter and compiler, and would lI think save quite a bit of memory.

@gbrail
Copy link
Collaborator

gbrail commented Jan 26, 2025

OK -- looks good to me -- thanks!

@gbrail gbrail merged commit ab43fc9 into mozilla:master Jan 26, 2025
3 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.

2 participants