WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1
2012-10-09 12:34:55 PDT
Created
attachment 167822
[details]
Patch
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
Comment on
attachment 167822
[details]
Patch
Attachment 167822
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/14250022
Mark Hahnenberg
Comment 4
2012-10-09 12:53:42 PDT
Created
attachment 167825
[details]
Patch
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
Comment on
attachment 167825
[details]
Patch
Attachment 167825
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/14223828
Mark Hahnenberg
Comment 7
2012-10-09 13:03:29 PDT
Created
attachment 167831
[details]
Patch
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
Comment on
attachment 167831
[details]
Patch
Attachment 167831
[details]
did not pass efl-ews (efl): Output:
http://queues.webkit.org/results/14249063
Mark Hahnenberg
Comment 10
2012-10-09 13:23:36 PDT
Created
attachment 167835
[details]
Patch
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
Created
attachment 167853
[details]
Patch
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
Created
attachment 168115
[details]
Patch
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
Created
attachment 168230
[details]
Patch
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
Created
attachment 168285
[details]
Patch
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
Committed
r131213
: <
http://trac.webkit.org/changeset/131213
>
Csaba Osztrogonác
Comment 23
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
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.
Top of Page
Format For Printing
XML
Clone This Bug