Bug 143210 – Logically empty WeakBlocks should not pin down their MarkedBlocks indefinitely.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andreas Kling]

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.

[Andreas Kling]

Created attachment 249734 [details]
Patch for EWS

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Andreas Kling]

Created attachment 249746 [details]
Proposed patch

[WebKit Commit Bot]

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.

[Andreas Kling]

Created attachment 249757 [details]
Patch

Style, build(EFL) fixes.

[Mark Lam]

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) ...".

[Geoffrey Garen]

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.

[Andreas Kling]

Created attachment 249790 [details]
Patch

Implement reviewer feedback. WeakBlock no longer detaches itself, but leaves that up to the caller.

[Geoffrey Garen]

Comment on attachment 249790 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 249790 [details]
Patch

Clearing flags on attachment: 249790

Committed r182200: <http://trac.webkit.org/changeset/182200>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143279

[Andreas Kling]

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.

[WebKit Commit Bot]

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

[Andreas Kling]

Committed r182347: <http://trac.webkit.org/changeset/182347>

[Simon Fraser (smfr)]

Is this not testable?