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.
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.
(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.
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); });
Created attachment 288574 [details] it begins for real Starting to turn the MarkedAllocator into a thingy that does bitvectors
Created attachment 288578 [details] a bit more
Created attachment 288595 [details] I think that I know how to allocate
Created attachment 288602 [details] sweeping is starting to make sense!
Created attachment 288690 [details] I think that MarkedBlock looks good
Created attachment 288699 [details] close to done
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.
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.
Created attachment 288716 [details] starting to compile
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.
Created attachment 288750 [details] more compiling
Created attachment 288757 [details] it compiles!
Created attachment 288855 [details] fixing it
Created attachment 288866 [details] it just ran a program
Created attachment 288976 [details] it ran splay
Created attachment 289068 [details] it passed testapi in debug mode
Created attachment 289080 [details] more It now has ChangeLogs.
Wow, it passes JSC tests! Let's see what EWS has to say.
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.
Something is wrong: earley 0.23629+-0.00409 ! 0.28332+-0.00153 ! definitely 1.1990x slower I'm investigating.
(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.
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.
Created attachment 289177 [details] looking quite nice indeed
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.
Created attachment 289178 [details] it should build
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.
Created attachment 289179 [details] performance Looks to be neutral on things that matter. I'll run in-browser benchmarks next.
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.
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
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.
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
Wow. I bet I messed up the incremental sweeper. I'll investigate later.
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.
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
Created attachment 289211 [details] fixed the crashes I was forgetting to set m_needsDestruction correctly.
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.
Created attachment 289212 [details] more fixes It passes tests for me. I think it just needs some #include love to build on all ports.
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.
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().
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.
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.
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.
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.
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.
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.
Created attachment 289306 [details] performance neutral on everything except Microbenchmarks.
Created attachment 289307 [details] more
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.
Created attachment 289364 [details] the patch
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.
Created attachment 289367 [details] the patch
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.
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.
(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.
Landed in http://trac.webkit.org/changeset/206154
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?
This looks like a 3-4% octane progression on macs.
(In reply to comment #60) > This looks like a 3-4% octane progression on macs. And on some iOS devices as well.