Bug 129986 – Reduce memory use for static property maps

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

Static property tables are currently duplicated on first use from readonly memory into dirty memory in every process, and since the entries are large (48 bytes) and the tables can be unusually sparse (we use a custom hash table without a rehash) a lot of memory may be wasted.

[Gavin Barraclough]

Created attachment 226255 [details]
Fix

[Gavin Barraclough]

No performance impact.

All benchmarks:
   <arithmetic>                                     177.3931+-0.5903     ?    177.4362+-0.3428        ? might be 1.0002x slower
   <geometric>                                       19.4142+-0.0491           19.3567+-0.0676          might be 1.0030x faster
   <harmonic>                                         4.3636+-0.0376            4.3354+-0.0453          might be 1.0065x faster

[WebKit Commit Bot]

Attachment 226255 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Lookup.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 226255 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=226255&action=review

This is looking pretty awesome, though Windows EWS doesn't seem impressed. :|

> Source/JavaScriptCore/runtime/JSObject.cpp:82
>          const HashTable* table = classInfo->propHashTable(exec);

Should use the propHashTable(VM&) overload here instead.

> Source/JavaScriptCore/runtime/JSObject.cpp:1628
> +                setUpStaticFunctionSlot(globalObject()->globalExec(), iter.value(), this, Identifier(&vm, iter.key()), slot);

Sidenote: We should make setUpStaticFunctionSlot() take a VM instead of an ExecState.

> Source/JavaScriptCore/runtime/Lookup.cpp:46
> +        keys = 0;

nullptr

> Source/JavaScriptCore/runtime/Lookup.h:186
> +                    return 0;

nullptr

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3973
> +    push(@implContent, "\nstatic const HashTableValue $nameEntries\[$packedSize\] =\n\{\n");

Do these end up in TEXT memory now?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4002
> -    push(@implContent, "static const HashTable $name = { $compactSize, $compactSizeMask, $hasSetter, $nameEntries, 0 };\n");
> +    push(@implContent, "static const HashTable $name = { $packedSize, $compactSizeMask, $hasSetter, $nameEntries, 0, $nameIndex };\n");

You're gonna need to rebaseline the WebCore bindings tests for these changes, you can do that by running "Tools/Scripts/run-bindings-tests --reset-results"

[Geoffrey Garen]

Comment on attachment 226255 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=226255&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:87
> +        for (HashTable::ConstIterator iter = table->begin(vm); iter != table->end(vm); ++iter) {
> +            if ((!(iter->attributes() & DontEnum) || (mode == IncludeDontEnumProperties)) && !((iter->attributes() & BuiltinOrFunction) && didReify))

This is probably a good place for "auto it = ..." instead of "HashTable::ConstIterator iter = ...".

[Geoffrey Garen]

Side note: Does the table still do perfect hashing (big table, no collisions)? If so, maybe we should undo that.

[Oliver Hunt]

(In reply to comment #6)
> Side note: Does the table still do perfect hashing (big table, no collisions)? If so, maybe we should undo that.

That's an interesting question, and probably worth investigating, especially now that more objects have hashtables.  I'm honestly not sure how space requirements change in the face same number of entries spread over more hash tables.

[Gavin Barraclough]

(In reply to comment #7)
> (In reply to comment #6)
> > Side note: Does the table still do perfect hashing (big table, no collisions)? If so, maybe we should undo that.
> 
> That's an interesting question, and probably worth investigating, especially now that more objects have hashtables.  I'm honestly not sure how space requirements change in the face same number of entries spread over more hash tables.

We don't create a perfect hash any more (not a change in this patch; most of the code is still there & even still runs, but results go unused!)

The table size is calculated here:

    sub calcCompactHashSize()
    {
        my $compactHashSize = ceilingToPowerOf2(2 * @keys);

So ~3x entries, with collisions chained.

Actually, after my change it might be a good idea to bring back perfect hashing. Previously it was expensive; since values were stored in the table it meant lots of wasted HashEntry space. Now it's just the index that would grow, and we'd halve the size of index entries (by removing the 'next' field used for chaining), which may pay for any growth.

[Gavin Barraclough]

(In reply to comment #4)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3973
> > +    push(@implContent, "\nstatic const HashTableValue $nameEntries\[$packedSize\] =\n\{\n");
> 
> Do these end up in TEXT memory now?

No, but I'm guessing this isn't actually a problem. The HashTableValues end up in a the __const section of the __DATA segment. I think the compiler can't prove that we don't ever cast away the constness & write to this, since we pass pointers around to HashTableValues. (This is all unchanged by this patch).

OOI the hash table index added in this patch does end up in __TEXT, so presumably the pointer to this table from the HashTable object is not enough to flummox the compiler!

Since this data all goes into the __const section I think it'll likely all end up remaining in undirtied file-backed COW pages, which should be just as good as __TEXT. If you want to test this theory, I think you can just add an attribute via the create_hash_table/CodeGeneratorJS.pm scripts:

    __attribute__ ((section ("__TEXT,__const")))

That should force the data into the __TEXT segment; my guess is that you won't see a change in private dirty memory from doing so.

[Andreas Kling]

Comment on attachment 226255 [details]
Fix

Looks like it's crashing the world on EWS :|

[Gavin Barraclough]

Created attachment 226349 [details]
Fix

[WebKit Commit Bot]

Attachment 226349 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Lookup.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226349 [details]
Fix

Attachment 226349 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5235945540419584

New failing tests:
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.clip.copy.html
canvas/philip/tests/2d.composite.canvas.copy.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.canvas.readonly.html
canvas/philip/tests/2d.composite.clip.destination-in.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.clip.destination-out.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect.shadow.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect.nonfinite.html

[Build Bot]

Created attachment 226355 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226349 [details]
Fix

Attachment 226349 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4834209197719552

New failing tests:
canvas/philip/tests/2d.composite.canvas.destination-in.html
canvas/philip/tests/2d.composite.canvas.source-out.html
canvas/philip/tests/2d.composite.clip.copy.html
canvas/philip/tests/2d.composite.canvas.copy.html
canvas/philip/tests/2d.clearRect.zero.html
canvas/philip/tests/2d.clearRect.globalalpha.html
canvas/philip/tests/2d.composite.canvas.source-in.html
canvas/philip/tests/2d.composite.canvas.destination-out.html
canvas/philip/tests/2d.clearRect.globalcomposite.html
canvas/philip/tests/2d.clearRect.clip.html
canvas/philip/tests/2d.composite.canvas.source-atop.html
canvas/philip/tests/2d.canvas.readonly.html
canvas/philip/tests/2d.composite.clip.destination-in.html
canvas/philip/tests/2d.composite.clip.destination-atop.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
canvas/philip/tests/2d.clearRect.transform.html
canvas/philip/tests/2d.clearRect.negative.html
canvas/philip/tests/2d.clearRect.path.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
canvas/philip/tests/2d.composite.canvas.destination-atop.html
canvas/philip/tests/2d.composite.canvas.destination-over.html
canvas/philip/tests/2d.composite.canvas.lighter.html
canvas/philip/tests/2d.composite.canvas.xor.html
canvas/philip/tests/2d.composite.canvas.source-over.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect.shadow.html
compositing/backface-visibility/backface-visibility-webgl.html
canvas/philip/tests/2d.clearRect.nonfinite.html

[Build Bot]

Created attachment 226358 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Gavin Barraclough]

Created attachment 226359 [details]
Fix

[WebKit Commit Bot]

Attachment 226359 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Lookup.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226359 [details]
Fix

Attachment 226359 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5865076644904960

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html

[Build Bot]

Created attachment 226374 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226359 [details]
Fix

Attachment 226359 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6146551621615616

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html

[Build Bot]

Created attachment 226377 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226359 [details]
Fix

Attachment 226359 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6194191398862848

New failing tests:
inspector-protocol/debugger/setPauseOnExceptions-all.html
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html

[Build Bot]

Created attachment 226383 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Gavin Barraclough]

Created attachment 226448 [details]
Fix

[Gavin Barraclough]

Transmitting file data ....................................
Committed revision 165482.

[Csaba Osztrogonác]

(In reply to comment #26)
> Transmitting file data ....................................
> Committed revision 165482.

It broke the Apple Windows build - http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/58955

[WebKit Commit Bot]

Re-opened since this is blocked by bug 130157

[Gavin Barraclough]

Created attachment 226565 [details]
Windows Fix

[Gavin Barraclough]

Transmitting file data ....................................
Committed revision 165603.

[Gavin Barraclough]

Followup fin in r165606.

[Csaba Osztrogonác]

(In reply to comment #30)
> Transmitting file data ....................................
> Committed revision 165603.

It broke the bindings-generation-tests. Could you fix it?