Currently a WeakBlock can keep its MarkedBlock alive forever if there is at least one Weak pointing at a WeakImpl inside the WeakBlock that's dead but not deallocated. This means that a single Weak object can keep 64kB(MarkedBlock) + 4kB(WeakBlock) alive indefinitely. My idea here is to detach WeakBlocks from their MarkedBlock once they have no pointers to live JSCells inside them. This will allow the garbage collector to reclaim their MarkedBlocks much sooner. The WeakBlocks will then be owned by the Heap and swept occasionally to make sure we destroy them too, once they're fully dead.
Created attachment 249734 [details] Patch for EWS
Attachment 249734 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/IncrementalSweeper.cpp:65: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1496: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249734 [details] Patch for EWS Attachment 249734 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6418867739951104 New failing tests: js/slow-stress/chain-custom-getter.html sputnik/Conformance/15_Native_Objects/15.8_Math/15.8.2/15.8.2.11_max/S15.8.2.11_A4.html sputnik/Implementation_Diagnostics/S14_D7.html js/regress/tear-off-arguments-simple.html sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A4.2.html tables/mozilla/marvin/tr_valign_top.html fast/loader/location-port.html sputnik/Conformance/15_Native_Objects/15.10_RegExp/15.10.6/15.10.6.2_RegExp.prototype.exec/S15.10.6.2_A4_T3.html webgl/1.0.2/conformance/ogles/GL/functions/functions_057_to_064.html sputnik/Unicode/Unicode_410/S7.6_A5.3_T2.html js/regress-139808.html svg/carto.net/frameless-svg-parse-error.html fast/animation/request-animation-frame-timestamps.html webgl/1.0.2/conformance/ogles/GL/sqrt/sqrt_001_to_006.html http/tests/security/contentSecurityPolicy/source-list-parsing-09.html canvas/philip/tests/2d.text.draw.baseline.alphabetic.html sputnik/Conformance/15_Native_Objects/15.7_Number/15.7.4/S15.7.4_A3.6.html sputnik/Unicode/Unicode_320/S7.6_A5.3_T2.html transitions/transition-end-event-destroy-iframe.html fast/encoding/script-in-head.html webgl/1.0.2/conformance/ogles/GL/lessThanEqual/lessThanEqual_001_to_008.html http/tests/security/isolatedWorld/storage-prototype.html
Created attachment 249741 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249734 [details] Patch for EWS Attachment 249734 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5692638162321408 Number of test failures exceeded the failure limit.
Created attachment 249742 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249746 [details] Proposed patch
Attachment 249746 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/IncrementalSweeper.cpp:64: Semicolon defining empty statement for this loop. Use { } instead. [whitespace/semicolon] [5] ERROR: Source/JavaScriptCore/heap/Heap.cpp:1496: Semicolon defining empty statement for this loop. Use { } instead. [whitespace/semicolon] [5] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 249757 [details] Patch Style, build(EFL) fixes.
Comment on attachment 249757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249757&action=review > Source/JavaScriptCore/ChangeLog:50 > + adding a new sweeping stage for the Heap's logically empty WeakBlocks. sweepNextBlock() is made "made" can be removed. > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:84 > + if (!didAnyWork) > + break; From what I can tell, you are inferring that if we didAnyWork, then there may be more work needed. Hence, we schedule the another timer cycle. Would it make sense instead to just do an explicit "if (!hasWork())" where bool hasWork() const { return !m_blocksToSweep.isEmpty() || !m_logicallyEmptyWeakBlocks.isEmpty(); } Then we don't have to schedule another timer cycle only to find the work lists empty just because the current cycle did some work. Personally, I also think "if (!hasWork()) break;" reads better in communicating the intent than "if (!didAnyWork) ...".
Comment on attachment 249757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249757&action=review Looks pretty good. I think I found a boog. Plus some suggestions for clarity. > Source/JavaScriptCore/heap/Heap.h:402 > + Vector<WeakBlock*> m_logicallyEmptyWeakBlocks; > + size_t m_indexOfNextLogicallyEmptyWeakBlockToSweep { WTF::notFound }; I think you need to clean this up inside lastChanceToFinalize, otherwise VM deallocation may leak memory. > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:88 > - while (!m_blocksToSweep.isEmpty()) { > - sweepNextBlock(); > + bool didAnyWork = false; > + for (;;) { > + bool didWork = sweepNextBlock(); > + didAnyWork |= didWork; > > double elapsedTime = WTF::monotonicallyIncreasingTime() - sweepBeginTime; > - if (elapsedTime < sweepTimeSlice) > + if (didWork && elapsedTime < sweepTimeSlice) > continue; > > + if (!didAnyWork) > + break; > + > scheduleTimer(); > return; > } I think you could simplify this: while (sweepNextBlock()) { double elapsedTime = WTF::monotonicallyIncreasingTime() - sweepBeginTime; if (elapsedTime < sweepTimeSlice) continue; scheduleTimer(); return; } > Source/JavaScriptCore/heap/WeakBlock.cpp:77 > + bool blockIsLogicallyEmpty = true; Seems like this probably belongs inside SweepResult. > Source/JavaScriptCore/heap/WeakBlock.cpp:102 > + // If this WeakBlock is logically empty, but still has Weaks pointing into it, > + // we can't destroy it just yet. Detach it from the WeakSet and hand ownership > + // to the Heap so we don't pin down the entire 64kB MarkedBlock. > + if (weakSet && !m_sweepResult.blockIsFree && blockIsLogicallyEmpty) { > + weakSet->detachBlock(this); > + weakSet->heap()->addLogicallyEmptyWeakBlock(this); > + } It looks like you're using the nullness or non-nullness of weakSet to indicate "I'm not going to use your sweep result, so it's OK to give it back to the heap". That's a bit subtle. Also, I think this code will remove from a list while iterating it. So, our caller will access block->next() after removing block from the list. That's a bit sketchy (even though it happens to work in practice). I think it might be better to structure this work so that the caller: (1) Loads the next item in the list at the head of the loop; (2) Checks the sweep result and removes the current block if it is logically empty but not free; (3) Moves on to the next item. That way, a WeakBlock only needs to know how to sweep itself, and it doesn't need to concern itself with what a client might do with the results, or how a client might organize its internal data.
Created attachment 249790 [details] Patch Implement reviewer feedback. WeakBlock no longer detaches itself, but leaves that up to the caller.
Comment on attachment 249790 [details] Patch r=me
Comment on attachment 249790 [details] Patch Clearing flags on attachment: 249790 Committed r182200: <http://trac.webkit.org/changeset/182200>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 143279
Comment on attachment 249790 [details] Patch I can't repro any assertions locally, so gonna try to reland and possibly kick a clean build if there's trouble.
Comment on attachment 249790 [details] Patch Rejecting attachment 249790 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 249790, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/5588581506613248
Committed r182347: <http://trac.webkit.org/changeset/182347>
Is this not testable?