Bug 98792 - Copying collection shouldn't require O(live bytes) memory overhead
Summary: Copying collection shouldn't require O(live bytes) memory overhead
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on: 99328 99331
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-09 10:37 PDT by Mark Hahnenberg
Modified: 2012-10-15 08:45 PDT (History)
7 users (show)

See Also:


Attachments
Patch (78.11 KB, patch)
2012-10-09 12:34 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.41 KB, patch)
2012-10-09 12:53 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.67 KB, patch)
2012-10-09 13:03 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.70 KB, patch)
2012-10-09 13:23 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (82.69 KB, patch)
2012-10-09 14:58 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.91 KB, patch)
2012-10-10 18:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.88 KB, patch)
2012-10-11 08:28 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (81.84 KB, patch)
2012-10-11 15:18 PDT, Mark Hahnenberg
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 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.
Comment 1 Mark Hahnenberg 2012-10-09 12:34:55 PDT
Created attachment 167822 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Gyuyoung Kim 2012-10-09 12:48:18 PDT
Comment on attachment 167822 [details]
Patch

Attachment 167822 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14250022
Comment 4 Mark Hahnenberg 2012-10-09 12:53:42 PDT
Created attachment 167825 [details]
Patch
Comment 5 WebKit Review Bot 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.
Comment 6 Gyuyoung Kim 2012-10-09 13:00:05 PDT
Comment on attachment 167825 [details]
Patch

Attachment 167825 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14223828
Comment 7 Mark Hahnenberg 2012-10-09 13:03:29 PDT
Created attachment 167831 [details]
Patch
Comment 8 WebKit Review Bot 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.
Comment 9 Gyuyoung Kim 2012-10-09 13:12:14 PDT
Comment on attachment 167831 [details]
Patch

Attachment 167831 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14249063
Comment 10 Mark Hahnenberg 2012-10-09 13:23:36 PDT
Created attachment 167835 [details]
Patch
Comment 11 WebKit Review Bot 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.
Comment 12 Mark Hahnenberg 2012-10-09 14:58:20 PDT
Created attachment 167853 [details]
Patch
Comment 13 WebKit Review Bot 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.
Comment 14 Mark Hahnenberg 2012-10-10 18:37:18 PDT
Created attachment 168115 [details]
Patch
Comment 15 WebKit Review Bot 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.
Comment 16 Geoffrey Garen 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.
Comment 17 Mark Hahnenberg 2012-10-11 08:28:51 PDT
Created attachment 168230 [details]
Patch
Comment 18 WebKit Review Bot 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.
Comment 19 Mark Hahnenberg 2012-10-11 15:18:11 PDT
Created attachment 168285 [details]
Patch
Comment 20 WebKit Review Bot 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.
Comment 21 Filip Pizlo 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.
Comment 22 Mark Hahnenberg 2012-10-12 12:38:33 PDT
Committed r131213: <http://trac.webkit.org/changeset/131213>
Comment 23 Csaba Osztrogonác 2012-10-15 04:30:56 PDT
(In reply to comment #22)
> Committed r131213: <http://trac.webkit.org/changeset/131213>

... and buildfixes landed in:
https://trac.webkit.org/changeset/131215
https://trac.webkit.org/changeset/131225
Comment 24 Csaba Osztrogonác 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
Comment 25 Mark Hahnenberg 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?
Comment 26 Mark Hahnenberg 2012-10-15 08:40:13 PDT
I filed bug 99331 as a potential fix for the ARM crashes.