RESOLVED FIXED 129986
Reduce memory use for static property maps
https://bugs.webkit.org/show_bug.cgi?id=129986
Summary Reduce memory use for static property maps
Gavin Barraclough
Reported 2014-03-08 20:13:15 PST
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.
Attachments
Fix (36.69 KB, patch)
2014-03-09 00:18 PST, Gavin Barraclough
kling: review-
Fix (91.16 KB, patch)
2014-03-10 16:42 PDT, Gavin Barraclough
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (156.77 KB, application/zip)
2014-03-10 17:47 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (216.69 KB, application/zip)
2014-03-10 18:05 PDT, Build Bot
no flags
Fix (107.02 KB, patch)
2014-03-10 18:18 PDT, Gavin Barraclough
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (498.66 KB, application/zip)
2014-03-10 19:43 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (721.59 KB, application/zip)
2014-03-10 20:10 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (531.72 KB, application/zip)
2014-03-10 21:27 PDT, Build Bot
no flags
Fix (123.87 KB, patch)
2014-03-11 16:46 PDT, Gavin Barraclough
no flags
Windows Fix (120.37 KB, patch)
2014-03-12 17:57 PDT, Gavin Barraclough
no flags
Gavin Barraclough
Comment 1 2014-03-09 00:18:29 PST
Gavin Barraclough
Comment 2 2014-03-09 00:19:04 PST
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
Comment 3 2014-03-09 00:20:10 PST
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 4 2014-03-09 00:57:30 PST
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 5 2014-03-09 12:19:56 PDT
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
Comment 6 2014-03-09 12:20:39 PDT
Side note: Does the table still do perfect hashing (big table, no collisions)? If so, maybe we should undo that.
Oliver Hunt
Comment 7 2014-03-09 13:01:17 PDT
(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
Comment 8 2014-03-09 16:23:32 PDT
(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
Comment 9 2014-03-09 16:34:33 PDT
(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 10 2014-03-09 19:48:54 PDT
Comment on attachment 226255 [details] Fix Looks like it's crashing the world on EWS :|
Gavin Barraclough
Comment 11 2014-03-10 16:42:22 PDT
WebKit Commit Bot
Comment 12 2014-03-10 16:43:34 PDT
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 13 2014-03-10 17:47:05 PDT
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
Comment 14 2014-03-10 17:47:15 PDT
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 15 2014-03-10 18:05:42 PDT
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
Comment 16 2014-03-10 18:05:52 PDT
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
Comment 17 2014-03-10 18:18:41 PDT
WebKit Commit Bot
Comment 18 2014-03-10 18:20:38 PDT
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 19 2014-03-10 19:43:15 PDT
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
Comment 20 2014-03-10 19:43:20 PDT
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 21 2014-03-10 20:10:35 PDT
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
Comment 22 2014-03-10 20:10:41 PDT
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 23 2014-03-10 21:27:26 PDT
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
Comment 24 2014-03-10 21:27:31 PDT
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
Comment 25 2014-03-11 16:46:42 PDT
Gavin Barraclough
Comment 26 2014-03-12 11:09:17 PDT
Transmitting file data .................................... Committed revision 165482.
Csaba Osztrogonác
Comment 27 2014-03-12 13:03:56 PDT
(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
Comment 28 2014-03-12 13:47:01 PDT
Re-opened since this is blocked by bug 130157
Gavin Barraclough
Comment 29 2014-03-12 17:57:09 PDT
Created attachment 226565 [details] Windows Fix
Gavin Barraclough
Comment 30 2014-03-13 23:47:05 PDT
Transmitting file data .................................... Committed revision 165603.
Gavin Barraclough
Comment 31 2014-03-14 01:16:21 PDT
Followup fin in r165606.
Csaba Osztrogonác
Comment 32 2014-03-14 01:50:33 PDT
(In reply to comment #30) > Transmitting file data .................................... > Committed revision 165603. It broke the bindings-generation-tests. Could you fix it?
Note You need to log in before you can comment on or make changes to this bug.