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.
Created attachment 226255 [details] Fix
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
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.
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"
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 = ...".
Side note: Does the table still do perfect hashing (big table, no collisions)? If so, maybe we should undo that.
(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.
(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.
(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.
Comment on attachment 226255 [details] Fix Looks like it's crashing the world on EWS :|
Created attachment 226349 [details] Fix
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.
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
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
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
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
Created attachment 226359 [details] Fix
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.
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
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
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
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
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
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
Created attachment 226448 [details] Fix
Transmitting file data .................................... Committed revision 165482.
(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
Re-opened since this is blocked by bug 130157
Created attachment 226565 [details] Windows Fix
Transmitting file data .................................... Committed revision 165603.
Followup fin in r165606.
(In reply to comment #30) > Transmitting file data .................................... > Committed revision 165603. It broke the bindings-generation-tests. Could you fix it?