Summary: | Disallow synchronous sweeping for eden GCs. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, fpizlo, ggaren, keith_miller, msaboff, saam | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Lam
2016-08-09 16:19:01 PDT
Actually, ... I can make collectAndSweep() work with EdenCollections with no perf impact. And so I shall do that and retain the current behavior of $vm.edenGC(). Created attachment 285704 [details]
proposed patch.
Comment on attachment 285704 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285704&action=review > Source/JavaScriptCore/heap/Heap.cpp:1095 > + // Sweep the list of blocks that the IncrementalSweeper would have swept later. > + while (!m_blockSnapshot.isEmpty()) { > + MarkedBlock* block = m_blockSnapshot.takeLast(); > + block->sweep(); > + objectSpace().freeOrShrinkBlock(block); > + } This is a layering violation. ObjectSpace should own any logic like this. For that reason, I think we should just disallow collectAndSweep in eden. Going back to disallowing collectAndSweep for EdenCollections. Created attachment 285785 [details]
proposed patch.
Comment on attachment 285785 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285785&action=review > Source/JavaScriptCore/heap/MarkedBlock.h:193 > + // will simply ignore Retured blocks (i.e. they will not be swept even Retired Comment on attachment 285785 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285785&action=review r=me > Source/JavaScriptCore/heap/MarkedBlock.cpp:-160 > case Allocated: > RELEASE_ASSERT_NOT_REACHED(); > - return FreeList(); Since all cases are handled in this switch, I think you can leave the return here and remove the assert and return at the end of the function. Comment on attachment 285785 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=285785&action=review Thanks for the review. Will fix the issues before landing. >> Source/JavaScriptCore/heap/MarkedBlock.cpp:-160 >> - return FreeList(); > > Since all cases are handled in this switch, I think you can leave the return here and remove the assert and return at the end of the function. Ok. Will do. >> Source/JavaScriptCore/heap/MarkedBlock.h:193 >> + // will simply ignore Retured blocks (i.e. they will not be swept even > > Retired Will fix. Landed in r204387: <http://trac.webkit.org/r204387>. (In reply to comment #8) > >> Source/JavaScriptCore/heap/MarkedBlock.cpp:-160 > >> - return FreeList(); > > > > Since all cases are handled in this switch, I think you can leave the return here and remove the assert and return at the end of the function. > > Ok. Will do. gcc did not like this. I restored the assertion and return statement at the end of the function. Fix landed in r204398: <http://trac.webkit.org/r204398>. |