WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Fix
(91.16 KB, patch)
2014-03-10 16:42 PDT
,
Gavin Barraclough
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
Fix
(107.02 KB, patch)
2014-03-10 18:18 PDT
,
Gavin Barraclough
buildbot
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Fix
(123.87 KB, patch)
2014-03-11 16:46 PDT
,
Gavin Barraclough
no flags
Details
Formatted Diff
Diff
Windows Fix
(120.37 KB, patch)
2014-03-12 17:57 PDT
,
Gavin Barraclough
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Gavin Barraclough
Comment 1
2014-03-09 00:18:29 PST
Created
attachment 226255
[details]
Fix
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
Created
attachment 226349
[details]
Fix
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
Created
attachment 226359
[details]
Fix
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
Created
attachment 226448
[details]
Fix
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.
Top of Page
Format For Printing
XML
Clone This Bug