Start to factor out slot map container

[aardvark179]

Start to factor out slot map container

This is a stacked PR on top of

• Reduce indirection and memory overhead for slot maps. #1782

This PR introduces an abstract super class of ScriptableObject and SlotMapContainer called SlotMapOwner. It is responsible for creating, getting, and setting the slot map owned by these objects. I originally experimented with this as an interface but was writing the almost exact same code for both classes and it was hard to limit the visibility of the API which should be entirely internal to Rhino. This also moves all slot promotion to the maps themselves. I did some minor refactoring in EmbeddedSlotMap to only check for promotion when would expand the backing array.

This changes the slot map structure in the single threaded case from

graph LR
Object["ScriptableObject"]
Container["SlotMapContainer"]
Map["SlotMap"]
Array["Slot[]"]
Slot1["Slot"]
Slot2["Slot"]
Slot3["Slot"]
Object1["ScriptableObject"]
Container1["SlotMapContainer"]
Map1["SlotMap"]
Array1["Slot[]"]
Slot4["Slot"]
Slot5["Slot"]
Slot6["Slot"]
Object --> Container --> Map --> Array
Array --> Slot1
Array --> Slot2
Array --> Slot3
Object1 --> Container1 --> Map1 --> Array1
Array1 --> Slot4
Array1 --> Slot5
Array1 --> Slot6

Loading

to:

graph LR
Object["ScriptableObject"]
Map["SlotMap"]
Array["Slot[]"]
Slot1["Slot"]
Slot2["Slot"]
Slot3["Slot"]
Object1["ScriptableObject"]
Map1["SlotMap"]
Array1["Slot[]"]
Slot4["Slot"]
Slot5["Slot"]
Slot6["Slot"]
Object --> Map --> Array
Array --> Slot1
Array --> Slot2
Array --> Slot3
Object1 --> Map1 --> Array1
Array1 --> Slot4
Array1 --> Slot5
Array1 --> Slot6

Loading

This saves 24 bytes per object.

[aardvark179]

Rebased after merge of #1782.

[gbrail]

Thanks for being patient -- I really like this work but I have two specific concerns that are making me reluctant to just move forward with this one. PTAL at my comments and can you let me know if you have any ideas? Thanks!

[gbrail]

I'm a bit concerned about making this volatile by default given how frequently it is accessed. Reading the docs for VarHandle, though, it looks like we can use it to choose volatile access semantics when we need them (in the thread-safe case) and non-volatile semantics otherwise. Do you know if that's something that we can do? I would be more comfortable with this if I could try to reduce as much overhead as possible in the single-threaded case.

[aardvark179]

I was expecting a question along those lines whether I made it volatile or not. I made it volatile as that was an easier way to think about things, but I believe it can be made non-volatile since all the multithreaded writes should be protected by fences. I'd like to think about this carefully before changing it though, and probably go through the §17.4 of the JLS and the VarHandles docs to make sure we aren't leaving any holes in the reasoning.

[gbrail]

Yes, I'd love to see if we can make this work with the same semantics without making that field volatile -- let me know what you learn about whether we can do that!

[aardvark179]

Okay. I'm happy that making the field non-volatile is okay. All access in multithreaded environments is either to immutable maps which are replaced with a CAS operation, or behind a lock.

[gbrail]

Is there a way for us to verify whether this feature is going to break Android compatibility? I think that our hypothesis is that it won't affect Android users unless they try to use "thread safe slot maps." However I don't know whether the incompatibility would be triggered by using the VarHandle, or merely by trying to load the class. Is there any way that we can verify this?

[aardvark179]

On Android 33 (Tiramisu) or later (released in 2022) VarHandles are supported so this should work fine. I am not sure what the behaviour is like before that (whether the class is present but returns an error, or if the app will fail at install or build time). Since we already suppressed warnings on ThreadSafeSlotMapContainer for android compatibility concerns would it be reasonable to move the VarHandle and uses to a separate class only used by the thread safe maps?

[gbrail]

I would be a lot more comfortable if the VarHandle stuff could be isolated to a class that's not loaded in the non-thread-safe case. It's too bad we don't have a clear compatibility policy for Android -- I've noticed that people have lots of versions that they use and it's hard to predict what will break.

[aardvark179]

Separated the multithreaded case to a separate class, and moved that to the later PR for thread safe stuff. I've kept it as a class in the same file so that it can have private access to the field.