Bug 92679 - Structures should be swept after all other objects
Summary: Structures should be swept after all other objects
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: 92700
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 14:25 PDT by Mark Hahnenberg
Modified: 2012-07-31 16:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.21 KB, patch)
2012-07-30 15:57 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2012-07-31 13:25 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-07-30 14:25:19 PDT
In order to get rid of ClassInfo from our objects, we need to be able to safely get the ClassInfo during the destruction of objects. We'd like to get the ClassInfo out of the Structure, but currently it is not safe to do so because the order of destruction of objects is not guaranteed to sweep objects before their corresponding Structure. We can fix this by sweeping Structures after everything else.
Comment 1 Mark Hahnenberg 2012-07-30 15:57:05 PDT
Created attachment 155389 [details]
Patch
Comment 2 Filip Pizlo 2012-07-30 17:26:50 PDT
Comment on attachment 155389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155389&action=review

> Source/JavaScriptCore/heap/HeapTimer.h:44
> +

Kill it with fire!

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:74
> +    return !m_blocksToSweep.size() || m_currentBlockToSweepIndex >= m_blocksToSweep.size();

I can has assert that m_currentBlockToSweepIndex <= m_blocksToSweep.size()
Comment 3 Mark Hahnenberg 2012-07-30 17:34:09 PDT
Committed r124123: <http://trac.webkit.org/changeset/124123>
Comment 4 Geoffrey Garen 2012-07-30 18:03:01 PDT
Comment on attachment 155389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155389&action=review

> Source/JavaScriptCore/heap/IncrementalSweeper.cpp:83
> +        MarkedBlock* block = m_blocksToSweep[m_currentBlockToSweepIndex];
> +        if (block->onlyContainsStructures()) {
> +            m_currentBlockToSweepIndex++;
> +            continue;

This logic gets a little whacky. Can't we just arrange for all the Structure blocks to be added to the array last, so we naturally sweep them last?
Comment 5 WebKit Review Bot 2012-07-30 18:37:53 PDT
Re-opened since this is blocked by 92700
Comment 6 Mark Hahnenberg 2012-07-31 13:25:20 PDT
Created attachment 155621 [details]
Patch
Comment 7 Mark Hahnenberg 2012-07-31 16:05:29 PDT
Committed r124265: <http://trac.webkit.org/changeset/124265>