RESOLVED FIXED 98792
Copying collection shouldn't require O(live bytes) memory overhead
https://bugs.webkit.org/show_bug.cgi?id=98792
Summary Copying collection shouldn't require O(live bytes) memory overhead
Mark Hahnenberg
Reported 2012-10-09 10:37:41 PDT
Currently our copying collection occurs simultaneously with the marking phase. We'd like to be able to reuse CopiedBlocks as soon as they become fully evacuated, but this is not currently possible because we don't know the liveness statistics of each old CopiedBlock until marking/copying has already finished. Instead, we have to allocate additional memory from the OS to use as our working set of CopiedBlocks while copying. We then return the fully evacuated old CopiedBlocks back to the block allocator, thus giving our copying phase an O(live bytes) overhead. To fix this, we should instead split the copying phase apart from the marking phase. This way we have full liveness data for each CopiedBlock during the copying phase so that we can reuse them the instant they become fully evacuated. With the additional liveness data that each CopiedBlock accumulates, we can add some additional heuristics to the collector. For example, we can calculate our global Heap fragmentation and only choose to do a copying phase if that fragmentation exceeds some limit. As another example, we can skip copying blocks that are already above a particular fragmentation limit, which allows older objects to coalesce into blocks that are rarely copied.
Attachments
Patch (78.11 KB, patch)
2012-10-09 12:34 PDT, Mark Hahnenberg
no flags
Patch (81.41 KB, patch)
2012-10-09 12:53 PDT, Mark Hahnenberg
no flags
Patch (81.67 KB, patch)
2012-10-09 13:03 PDT, Mark Hahnenberg
no flags
Patch (81.70 KB, patch)
2012-10-09 13:23 PDT, Mark Hahnenberg
no flags
Patch (82.69 KB, patch)
2012-10-09 14:58 PDT, Mark Hahnenberg
no flags
Patch (81.91 KB, patch)
2012-10-10 18:37 PDT, Mark Hahnenberg
no flags
Patch (81.88 KB, patch)
2012-10-11 08:28 PDT, Mark Hahnenberg
no flags
Patch (81.84 KB, patch)
2012-10-11 15:18 PDT, Mark Hahnenberg
fpizlo: review+
Mark Hahnenberg
Comment 1 2012-10-09 12:34:55 PDT
WebKit Review Bot
Comment 2 2012-10-09 12:38:52 PDT
Attachment 167822 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 3 2012-10-09 12:48:18 PDT
Mark Hahnenberg
Comment 4 2012-10-09 12:53:42 PDT
WebKit Review Bot
Comment 5 2012-10-09 12:56:13 PDT
Attachment 167825 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 6 2012-10-09 13:00:05 PDT
Mark Hahnenberg
Comment 7 2012-10-09 13:03:29 PDT
WebKit Review Bot
Comment 8 2012-10-09 13:07:33 PDT
Attachment 167831 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 9 2012-10-09 13:12:14 PDT
Mark Hahnenberg
Comment 10 2012-10-09 13:23:36 PDT
WebKit Review Bot
Comment 11 2012-10-09 13:25:49 PDT
Attachment 167835 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Hahnenberg
Comment 12 2012-10-09 14:58:20 PDT
WebKit Review Bot
Comment 13 2012-10-09 15:00:42 PDT
Attachment 167853 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Hahnenberg
Comment 14 2012-10-10 18:37:18 PDT
WebKit Review Bot
Comment 15 2012-10-10 18:41:06 PDT
Attachment 168115 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Geoffrey Garen
Comment 16 2012-10-10 22:57:04 PDT
Comment on attachment 168115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168115&action=review > Source/JavaScriptCore/runtime/Options.h:120 > + v(double, maxCopiedSpaceFragmentation, 0.8) \ > + v(double, maxCopiedBlockFragmentation, 0.9) \ I believe the right word here is "min...Utilization". "Fragmentation" usually means the wasted space, not the used space.
Mark Hahnenberg
Comment 17 2012-10-11 08:28:51 PDT
WebKit Review Bot
Comment 18 2012-10-11 08:32:16 PDT
Attachment 168230 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Hahnenberg
Comment 19 2012-10-11 15:18:11 PDT
WebKit Review Bot
Comment 20 2012-10-11 15:19:43 PDT
Attachment 168285 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1 Source/JavaScriptCore/runtime/JSCell.h:41: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filip Pizlo
Comment 21 2012-10-11 16:18:58 PDT
Comment on attachment 168285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168285&action=review > Source/JavaScriptCore/heap/GCThread.cpp:88 > + { > + MutexLocker locker(m_shared.m_markingLock); > + } Add a comment to say why this trick works. > Source/JavaScriptCore/heap/GCThread.cpp:94 > + m_slotVisitor->drainFromShared(SlotVisitor::SlaveDrain); One sentence comment about termination condition for marking, please. > Source/JavaScriptCore/heap/GCThread.cpp:98 > + m_copyVisitor->copyFromShared(); One sentence comment about termination condition for copying, please. > Source/JavaScriptCore/heap/Heap.h:267 > + struct CopyFunctor : public MarkedBlock::VoidFunctor { Up to you, but you could rename it to something like MarkedBlockSnapshotFunctor.
Mark Hahnenberg
Comment 22 2012-10-12 12:38:33 PDT
Csaba Osztrogonác
Comment 23 2012-10-15 04:30:56 PDT
Csaba Osztrogonác
Comment 24 2012-10-15 08:06:25 PDT
(In reply to comment #22) > Committed r131213: <http://trac.webkit.org/changeset/131213> And it made all tests crash on Qt ARM - https://bugs.webkit.org/show_bug.cgi?id=99328
Mark Hahnenberg
Comment 25 2012-10-15 08:30:45 PDT
(In reply to comment #24) > (In reply to comment #22) > > Committed r131213: <http://trac.webkit.org/changeset/131213> > > And it made all tests crash on Qt ARM - https://bugs.webkit.org/show_bug.cgi?id=99328 It's odd that it only made ARMv7 fail. I added a new use of WTF::weakCompareAndSwap which would be called during the marking phase in CopiedBlock::reportLiveBytes. Maybe that's what's causing the crash if ARMv7 doesn't support CAS?
Mark Hahnenberg
Comment 26 2012-10-15 08:40:13 PDT
I filed bug 99331 as a potential fix for the ARM crashes.
Note You need to log in before you can comment on or make changes to this bug.