383592 - Switch to HashMap for ScriptableObject property storage

Status: RESOLVED INACTIVE

(Rhino Graveyard :: Core, defect)

Description

[Norris Boyd]

In bug 360964 it was proposed that we switch to HashMap for Scriptable object property storage, and maybe even implement some sort of pluggable architecture.

Comment 1

[Attila Szegedi]

Yeah, a pluggable architecture would be in order, in particular to give people using objects from multiple threads more control -- I can imagine some people would want a simple hashmap, others a synchronized one, yet others could use a java.util.ConcurrentHashMap. Of course, this also might go into the issues of explicit thread-safety measures within the ScriptableObject...

Comment 2

[Marc]

#412928 contains a patch providing the needed refactoring.

Comment 3

[Norris Boyd]

I worked on this a while, trying various optimizations, but I couldn't find a way to switch to standard Map classes without a significant performance penalty. 

I believe the main need for this change was to maintain the insertion order of properties for iteration (see bug 419090), so I've just checked in a change that accomplishes this by modifying the current Rhino ScriptableObject implementation rather than replacing it. This change has minimal performance impact.

So I'm thinking I won't fix this bug, at least anytime soon. There's some value in a pluggable Map implementation, but the runtime cost is surprisingly high.

Comment 4

[Mario Cormier]

Note that ScriptableObject now implements java.util.Map (see bug 448816), which might be of interest to some people looking at this bug.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Closing. Bug management is now done here:
https://github.com/mozilla/rhino