Bug 161581 – Make MarkedBlock state tracking support overlapped allocation and marking state

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

For example, each MarkedAllocator could have a Vector<std::unique_ptr<MarkedBlock::Handle>> m_blocks.  Then it could have the following sets represented by bitvectors indexed by the handle's index in m_blocks:
- m_canAllocate, which contains all non-empty non-retired blocks.
- m_snapshot, which contains all blocks in the block snapshot.
- m_empty, which contains all empty blocks.
- m_eden, which contains all blocks that have new objects.
- m_allocated, which contains all blocks that have been completely filled up.

If we do this, then we can get rid of block state, the m_blocksWithNewObjects vector, and the block snapshot vector.  The following operations become cheap operations over bitvectors:

- All of the flips:
    - Full GC flip: m_empty = 0, m_canAllocate = 0, m_allocated = 0.
    - Eden GC flip: m_canAllocate |= m_eden, m_allocated = 0.
    - Full GC snapshot: m_snapshot = 1.
    - Eden GC snapshot: m_snapshot |= m_eden.

- All of the iterations:
    - Allocate: scan m_canAllocate, then scan m_empty, then scan m_empty of other size classes (so we can rebalance the heap cheaply!).
    - Incremental sweep: scan (m_snapshot & ~m_allocated).

We could allocate WeakSets outside MarkedBlocks, and have them also be tracked in their own Vector<std::unique_ptr<WeakSet>>, owned by MarkedSpace, and also track various sets of them using bitvectors.

This ought to reduce any per-block overheads by so much that they should appear invisible.  We did lose some perf by going from 64KB to 16KB even with many optimizations, so this may be enough to bring us back to where we were before.  For example, consider what happens when we are using 200MB.  That means having to sometimes consider about 13000 blocks, which the current code does by looping over that many things, and those things are unlikely to reside on the same cache line.  But with bitvectors, the typical flipping operation would only have to touch 13000 / 8 = 1625 bytes of contiguous bits.  Using 64-bit operations, that means only having to loop over about 200 words.  That means that the previous flipping operations that would take about 13000 iterations just to touch each block will now only require 200 iterations - as in we actually have a chance of speeding up those operations by 65x.

[Filip Pizlo]

Another benefit of this approach is that on-the-fly retirement during marking will now be a CAS to clear the can-allocate bit rather than some crazy locking.

Maybe we can even make the clearing of the can-allocate bit racy.  In the worst case, we would forget that we had retired some blocks.  That's not the end of the world.

[Filip Pizlo]

(In reply to comment #0)
> require 200 iterations - as in we actually have a chance of speeding up
> those operations by 65x.

It's actually speeding up by exactly 64x.

[Filip Pizlo]

Created attachment 288519 [details]
it begins

I'm starting out by creating a super-flexible bitvector framework.  What I have now will make FastBitVector capable of the following amazing stuff:

FastBitVector a;
FastBitVector b;
FastBitVector c;
... // do things to populate a, b, c
auto intersection = a.bitAnd(b); // Returns a view of a & b that is computed on the fly.
intersection.forEachSetBit([&] (size_t i) { print(i); }); // Super efficiently walks the words of a & b.
a.bitAnd(b.bitOr(c).bitNot()).forEachClearBit([&] (size_t i) { print (i); });

[Filip Pizlo]

Created attachment 288574 [details]
it begins for real

Starting to turn the MarkedAllocator into a thingy that does bitvectors

[Filip Pizlo]

Created attachment 288578 [details]
a bit more

[Filip Pizlo]

Created attachment 288595 [details]
I think that I know how to allocate

[Filip Pizlo]

Created attachment 288602 [details]
sweeping is starting to make sense!

[Filip Pizlo]

Created attachment 288690 [details]
I think that MarkedBlock looks good

[Filip Pizlo]

Created attachment 288699 [details]
close to done

[Filip Pizlo]

Created attachment 288702 [details]
holy cow it might be done

This patch turned out so wonderful.  I think I've axed all of the block scans in the GC!

And look what we're left with.  For example, setting up the block states after marking:

    m_empty = m_live & ~m_markingNotEmpty;
    m_canAllocate = m_live & m_markingNotEmpty & ~m_markingRetired;

Or snapshotting in an eden collection:

    m_snapshot |= m_eden;

It's like garbage poetry.

[Filip Pizlo]

Created attachment 288714 [details]
more things

I made a few changes after I thought through how a collection cycle really works.

I added a big comment that illustrates how the various bits work.

[Filip Pizlo]

Created attachment 288716 [details]
starting to compile

[Filip Pizlo]

I changed the title of the bug to reflect the most significant end goal that this change achieves.

Even if it was perf-neutral, we'd still take it since unlike trunk, this would take us a big step closer to allowing allocation to happen during marking.

[Filip Pizlo]

Created attachment 288750 [details]
more compiling

[Filip Pizlo]

Created attachment 288757 [details]
it compiles!

[Filip Pizlo]

Created attachment 288855 [details]
fixing it

[Filip Pizlo]

Created attachment 288866 [details]
it just ran a program

[Filip Pizlo]

Created attachment 288976 [details]
it ran splay

[Filip Pizlo]

Created attachment 289068 [details]
it passed testapi in debug mode

[Filip Pizlo]

Created attachment 289080 [details]
more

It now has ChangeLogs.

[Filip Pizlo]

Wow, it passes JSC tests!  Let's see what EWS has to say.

[WebKit Commit Bot]

Attachment 289080 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:244:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:254:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:141:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 5 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Something is wrong:

   earley                                            0.23629+-0.00409    !     0.28332+-0.00153       ! definitely 1.1990x slower

I'm investigating.

[Filip Pizlo]

(In reply to comment #23)
> Something is wrong:
> 
>    earley                                            0.23629+-0.00409    !  
> 0.28332+-0.00153       ! definitely 1.1990x slower
> 
> I'm investigating.

Ha!  Found two issues:

1) When we moved a MarkedBlock from one allocator to another, we weren't fixing up its cellSize and attributes.  I have no idea why this didn't cause a ton of test failures.  I fixed it but now I'm going to have to write some kind of test for it.  Though this wasn't the reason for the perf regression - I just found it while investigating.

2) The reason for the regression was that we were first allocating out of non-empty blocks and then allocating out of empty blocks. In practice this meant that we would spend more time overall allocating out of non-empty blocks, which meant doing more popping and less bumping.  It also seemed to create more fragmentation overall.  This was so easy to fix.  I just switched:

        m_allocationCursor = m_canAllocate.findBit(m_allocationCursor, true);

to this:

        m_allocationCursor = (m_canAllocate | m_empty).findBit(m_allocationCursor, true);

and voila, no regression on earley.

Still running other tests.

[Filip Pizlo]

Created attachment 289175 [details]
fixed the earley regression, got what might be a splay progression!

Replaced the best-fit search (canAllocate first, empty second) with a first-fit search (scan both canAllocate and empty in tandem).  This seems to work a lot better.

Also fixed a bug where we weren't properly reassigning blocks between allocators.

Also made lookForEmptyBlocksInOtherAllocators into an Option.  It's great for debugging, since this feature is 100% optional for correctness and it's a super likely source of bugs.

[Filip Pizlo]

Created attachment 289177 [details]
looking quite nice indeed

[Filip Pizlo]

I messed with how WeakInlines.h gets included because it was causing a nasty header cycle, so this is going to require more header rewiring in the DOM.  I'm going to do that next.

[Filip Pizlo]

Created attachment 289178 [details]
it should build

[WebKit Commit Bot]

Attachment 289178 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:244:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:254:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289179 [details]
performance

Looks to be neutral on things that matter.

I'll run in-browser benchmarks next.

[Build Bot]

Comment on attachment 289178 [details]
it should build

Attachment 289178 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2096606

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289180 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289178 [details]
it should build

Attachment 289178 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2096616

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289181 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Filip Pizlo]

Wow. I bet I messed up the incremental sweeper. I'll investigate later.

[Build Bot]

Comment on attachment 289178 [details]
it should build

Attachment 289178 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2096618

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 289182 [details]
Archive of layout-test-results from ews121 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Filip Pizlo]

Created attachment 289211 [details]
fixed the crashes

I was forgetting to set m_needsDestruction correctly.

[WebKit Commit Bot]

Attachment 289211 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289212 [details]
more fixes

It passes tests for me.  I think it just needs some #include love to build on all ports.

[WebKit Commit Bot]

Attachment 289212 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289232 [details]
fixed looking for empty blocks

The feature to look for empty blocks in other allocators didn't actually work, because MarkedSpace::m_allocatorForEmptyAllocation was stuck null.  Fixing that without doing anything else causes a splay regression and divergent memory usage (1GB after a couple seconds instead of steady at 200MB) because we didn't check for the need to collect until we failed in tryAllocateWithoutCollecting().  So, this also changes allocateSlowCaseImpl() to always check for the need to collect before doing tryAllocateWithoutCollecting().

[WebKit Commit Bot]

Attachment 289232 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289238 [details]
the patch

Still running experiments, but I think it's ready for review.

Note that some benchmarks regress when I enable searching for empty blocks, while others progress.  If I disable it, then this is perf-neutral.  That part of this patch is not on the critical path for concurrent GC.  I'll land with it either turned on or off depending on what all of the benchmarks say.

[Filip Pizlo]

Created attachment 289239 [details]
performance

Short programs that allocate lots of strings seem not to like searching for empty blocks in other allocators or the change to trigger GC as soon as the eden size is consumed.  Larger programs don't seem to mind this.  I'm going to test in-browser next to get a definitive answer.

[WebKit Commit Bot]

Attachment 289238 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289291 [details]
more

I've made some changes to deal with a PLT regression.  I think I've got it under control now.  Now I'll test it.

[WebKit Commit Bot]

Attachment 289291 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289306 [details]
performance

neutral on everything except Microbenchmarks.

[Filip Pizlo]

Created attachment 289307 [details]
more

[WebKit Commit Bot]

Attachment 289307 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289364 [details]
the patch

[WebKit Commit Bot]

Attachment 289364 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 289367 [details]
the patch

[WebKit Commit Bot]

Attachment 289367 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:241:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedSpace.cpp:251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:83:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:85:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/WeakGCMap.h:96:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:192:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:297:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/heap/MarkedAllocator.h:156:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/Scripts/builtins/builtins_generate_internals_wrapper_header.py:65:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 9 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 289367 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289367&action=review

r=me

> Source/JavaScriptCore/ChangeLog:81
> +        Browser benchmarks:

Did the rebalancing feature produce a high watermark memory reduction in the end on v8-splay or v8-earley?

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:349
> +        // FIXME: Make this work again.
> +        // https://bugs.webkit.org/show_bug.cgi?id=162296

Should probably RELEASE_ASSERT here so someone doesn't fall into a trap.

> Source/JavaScriptCore/heap/MarkedAllocator.h:84
> +// - by default we have clear markingNotEmpty/markingRetired bits.

cleared?

> Source/JavaScriptCore/heap/MarkedAllocator.h:134
> +// After this, we just need to answer the question: what does an object allocated during eden GC
> +// look like to the GC?
> +//
> +// I've come up with two compelling answers.

I think the paragraphs below belong better in a comment on a concurrent GC bugzilla or in the ChangeLog. For the time being, they don't describe the behavior of the code.

> Source/JavaScriptCore/heap/MarkedAllocator.h:145
> +// we'd make the object is eligible for freeing at the next eden GC. There's a bunch of things we

is eligible

> Source/JavaScriptCore/heap/MarkedSpace.cpp:241
> +        //dataLog("Allocated ", RawPointer(result), "\n");

Revert.

> Source/JavaScriptCore/heap/MarkedSpace.cpp:251
> +        //dataLog("Try-allocated ", RawPointer(result), "\n");

Revert.

> Source/JavaScriptCore/heap/SlotVisitor.cpp:297
> +    //dataLog("Visiting ", RawPointer(cell), "\n");

Revert.

[Filip Pizlo]

(In reply to comment #56)
> Comment on attachment 289367 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=289367&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:81
> > +        Browser benchmarks:
> 
> Did the rebalancing feature produce a high watermark memory reduction in the
> end on v8-splay or v8-earley?

It did.  It reduces the heap capacity in splay from ~200KB to ~180KB.  I'll note it in the changelog.

> 
> > Source/JavaScriptCore/heap/MarkedAllocator.cpp:349
> > +        // FIXME: Make this work again.
> > +        // https://bugs.webkit.org/show_bug.cgi?id=162296
> 
> Should probably RELEASE_ASSERT here so someone doesn't fall into a trap.

Good idea.

> 
> > Source/JavaScriptCore/heap/MarkedAllocator.h:84
> > +// - by default we have clear markingNotEmpty/markingRetired bits.
> 
> cleared?

Yup.

> 
> > Source/JavaScriptCore/heap/MarkedAllocator.h:134
> > +// After this, we just need to answer the question: what does an object allocated during eden GC
> > +// look like to the GC?
> > +//
> > +// I've come up with two compelling answers.
> 
> I think the paragraphs below belong better in a comment on a concurrent GC
> bugzilla or in the ChangeLog. For the time being, they don't describe the
> behavior of the code.

Removed.

> 
> > Source/JavaScriptCore/heap/MarkedAllocator.h:145
> > +// we'd make the object is eligible for freeing at the next eden GC. There's a bunch of things we
> 
> is eligible
> 
> > Source/JavaScriptCore/heap/MarkedSpace.cpp:241
> > +        //dataLog("Allocated ", RawPointer(result), "\n");
> 
> Revert.
> 
> > Source/JavaScriptCore/heap/MarkedSpace.cpp:251
> > +        //dataLog("Try-allocated ", RawPointer(result), "\n");
> 
> Revert.
> 
> > Source/JavaScriptCore/heap/SlotVisitor.cpp:297
> > +    //dataLog("Visiting ", RawPointer(cell), "\n");
> 
> Revert.

I removed these.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/206154

[Saam Barati]

Comment on attachment 289367 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289367&action=review

> Source/JavaScriptCore/heap/Heap.cpp:1079
> +    if (false) {

Is it worth putting this under logGC?

[Saam Barati]

This looks like a 3-4% octane progression on macs.

[Saam Barati]

(In reply to comment #60)
> This looks like a 3-4% octane progression on macs.

And on some iOS devices as well.