Bug 143210 - Logically empty WeakBlocks should not pin down their MarkedBlocks indefinitely.
Summary: Logically empty WeakBlocks should not pin down their MarkedBlocks indefinitely.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on: 143279
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-29 21:50 PDT by Andreas Kling
Modified: 2015-04-04 16:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch for EWS (10.82 KB, patch)
2015-03-30 09:14 PDT, Andreas Kling
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (1010.05 KB, application/zip)
2015-03-30 10:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (1010.08 KB, application/zip)
2015-03-30 10:35 PDT, Build Bot
no flags Details
Proposed patch (12.84 KB, patch)
2015-03-30 10:52 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (12.88 KB, patch)
2015-03-30 13:17 PDT, Andreas Kling
ggaren: review-
Details | Formatted Diff | Diff
Patch (12.68 KB, patch)
2015-03-30 18:31 PDT, Andreas Kling
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-03-29 21:50:25 PDT
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.
Comment 1 Andreas Kling 2015-03-30 09:14:56 PDT
Created attachment 249734 [details]
Patch for EWS
Comment 2 WebKit Commit Bot 2015-03-30 09:16:41 PDT
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 3 Build Bot 2015-03-30 10:30:13 PDT
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
Comment 4 Build Bot 2015-03-30 10:30:17 PDT
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 5 Build Bot 2015-03-30 10:35:34 PDT
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.
Comment 6 Build Bot 2015-03-30 10:35:40 PDT
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
Comment 7 Andreas Kling 2015-03-30 10:52:55 PDT
Created attachment 249746 [details]
Proposed patch
Comment 8 WebKit Commit Bot 2015-03-30 10:55:29 PDT
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.
Comment 9 Andreas Kling 2015-03-30 13:17:20 PDT
Created attachment 249757 [details]
Patch

Style, build(EFL) fixes.
Comment 10 Mark Lam 2015-03-30 14:48:00 PDT
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 11 Geoffrey Garen 2015-03-30 15:05:44 PDT
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.
Comment 12 Andreas Kling 2015-03-30 18:31:23 PDT
Created attachment 249790 [details]
Patch

Implement reviewer feedback. WeakBlock no longer detaches itself, but leaves that up to the caller.
Comment 13 Geoffrey Garen 2015-03-31 11:02:38 PDT
Comment on attachment 249790 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2015-03-31 13:02:17 PDT
Comment on attachment 249790 [details]
Patch

Clearing flags on attachment: 249790

Committed r182200: <http://trac.webkit.org/changeset/182200>
Comment 15 WebKit Commit Bot 2015-03-31 13:02:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Commit Bot 2015-03-31 15:41:24 PDT
Re-opened since this is blocked by bug 143279
Comment 17 Andreas Kling 2015-04-02 13:58:21 PDT
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 18 WebKit Commit Bot 2015-04-02 14:00:06 PDT
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
Comment 19 Andreas Kling 2015-04-04 16:17:14 PDT
Committed r182347: <http://trac.webkit.org/changeset/182347>
Comment 20 Simon Fraser (smfr) 2015-04-04 16:20:06 PDT
Is this not testable?