412928 - Refactor ScriptableObject to use a Map rather than self made structure to save slots

Status: RESOLVED INACTIVE

(Rhino Graveyard :: Core, enhancement)

Description

[Marc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: 

ScriptableObject uses a self made structure to save the property slots (legagy code?). It should be refactored to use a Map.

Provided patch fixes this using a LinkedHashMap. Execution speed seems to be comparable both for Rhino "normal tests" as for HtmlUnit test suite which is surely more representative of a "normal" usage.

The provided patch doesn't address the question of thread safety and the possibility to configure it but this could be done.

ScriptableObject instances serialized with a previous version of Rhino can't be de-serialized with this new version. Compatibility could probably be added but I doubt that it makes sense.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marc]

Attachment: the patch to use a Map in ScriptableObject

Comment 2

[Norris Boyd]

Looking at this now. 

Memory usage:
Running the following script

var i=0;
var a = {};
for (;;) {
  a[i++] = i;
  if (i % 100 == 0) print(i);
}

until it runs out of memory using default JDK 1.6 settings, I get the following values:

current: 1179600
with patch: 707500

So the version with a patch can allocate 40% fewer indices. 

This benchmark could be skewed by the allocation of Integers for the indices, so I tried the following:

var i=0;
var a = {};
for (;;) {
  var name = "a" + i++;
  a[name] = name;
  if (i % 100 == 0) print(i);
}

Results:
current: 692900
with patch: 563100

Now the difference is about 20% fewer properties allocated.

To test speed I created this benchmark:
var obj = { a:1, b:2, c:3 };
var d = new Date();
(function () {
  for (var i=0; i < 1000000; i++) {
    var t = obj.a;
    obj.a = obj.b;
    obj.b = obj.c
    obj.a = t;
  }
})();
print(new Date() - d);

Timings (avg of 4 runs):
current: 1756
with patch: 2171

So over 20% slower.

Additionally, the current implementation provides some level of thread safety (I know now that some of the locking idioms used are not safe at least in some circumstances, but people have been relying on the locking in their usages). I changed from using a LinkedHashMap to Collections.synchronizedMap(new LinkedHashMap()) and got these results:

maxIndex.js: 563000 (no change?!)
propertyAccess.js timing: 3558.5 (2X current)

For comparison I tried using just HashMap:

maxIndex.js: 606400 (87.5% of current; 12.5% worse)
propertyAccess.js timing: 1882 (only 7% worse than current)

And then Collections.synchronizedMap(new HashMap()):

maxIndex.js: 606400 (87.5% of current; 12.5% worse)
propertyAccess.js timing: 3397.25 (almost 2X worse than current)

Comment 3

[Marc]

Does it mean that the performance of the Map based properties storage is too bad for you?

Comment 4

[Norris Boyd]

Attachment: 412928.patch

Here's my version of the patch, including a MapFactory interface to allow embeddings to control the type of map used.

Comment 5

[Norris Boyd]

(In reply to comment #3)
> Does it mean that the performance of the Map based properties storage is too
> bad for you?
> 

It means that I want to look at it more closely; perhaps there are some optimizations implemented in the current version that we need to bring forward. 

Comment 6

[Marc]

a MapFactory would be ok for me too.

What about packing current low level implementation into a custom Map that would be the default? This should allow to use a Map as well as to keep current speed?

What is with thread safety?

Comment 7

[Daniel Gredler]

Just to kick the ant heap a little... ;-)

I've posted a patch at bug 419090 which implements definition-order property enumerations using a doubly-linked list (in addition to the current slot data structure, which is kept for fast lookups).

I've run the performance tests posted by Norris above, and get the following results:

Memory usage (ints): 9% fewer properties can be defined
Memory usage (strings): 14% fewer properties can be defined
Speed: 1% slower

Thoughts?

Comment 8

[Ahmed Ashour]

Any updates on this, please?

Comment 9

[Norris Boyd]

Attachment: Updated patch

An updated patch as I've tried various things to improve performance. I want to investigate modifying the current scheme rather than switching to a map as performance is still pretty significantly degraded, but want to save the current changes for future reference.

Comment 10

[Norris Boyd]

Attachment: Results from running js-speed on current CVS rhino without patch

Comment 11

[Norris Boyd]

Attachment: Results from running js-speed on current CVS rhino *with* patch

Get performance tests using 
   svn co http://ejohn.org/code/js-speed/

total time, about 51% slower.

Comment 12

[Daniel Gredler]

@Norris: If you're looking into improving the current scheme, take a look at the patch attached to bug 419090 -- the performance impact appeared to be minimal (see my comment above).

Comment 13

[Sylvestre Ledru [:Sylvestre]]

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