Summary: | MarkedAllocator::tryAllocateHelper() should sweep another block if it can't sweep a Structure block | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | fpizlo, ggaren | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mark Hahnenberg
2012-07-31 18:22:09 PDT
Created attachment 155698 [details]
Patch
Comment on attachment 155698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review Looks good but I think I see a bug. > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 > + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { I don't think you want this loop here, do you? The goal of this function is to sweep just one. > Source/JavaScriptCore/heap/MarkedAllocator.cpp:38 > + m_heap->sweeper()->sweepNextBlock(); This could use a "why" comment. (In reply to comment #2) > (From update of attachment 155698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review > > Looks good but I think I see a bug. > > > Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 > > + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { > > I don't think you want this loop here, do you? The goal of this function is to sweep just one. The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). > This could use a "why" comment. Will do. > The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false).
That being said, I'll come up with a better way to write it.
Comment on attachment 155698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review >>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 >>> + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { >> >> I don't think you want this loop here, do you? The goal of this function is to sweep just one. > > The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping. Comment on attachment 155698 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155698&action=review Please add the "why" comment before landing. >>>> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:92 >>>> + while (m_currentBlockToSweepIndex < m_blocksToSweep.size()) { >>> >>> I don't think you want this loop here, do you? The goal of this function is to sweep just one. >> >> The loop needs to be there in case the next block doesn't need sweeping (i.e. block->needsSweeping() == false). > > OK, I see where you're going with that. I still think it would be simpler (and slightly faster) to remove the extra loop. I see sweeping a block with no destructors as just a special fast case, and not a reason to keep sweeping. Eh, you convinced me. Let's keep it this way to keep things simple. Committed r124352: <http://trac.webkit.org/changeset/124352> |