Bug 160716 – Disallow synchronous sweeping for eden GCs.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This functionality was broken, and it was only used by $vm.edenGC().  It's sad that we can't do a synchronous sweep after an edenGC for testing purposes anymore, but as far as I know, this functionality hasn't been used much.  So, I'm removing it for now and switching $vm.edenGC() do the default eden GC behavior which lets the incremental sweeper do the sweeping.

[Mark Lam]

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().

[Mark Lam]

Created attachment 285704 [details]
proposed patch.

[Filip Pizlo]

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.

[Mark Lam]

Going back to disallowing collectAndSweep for EdenCollections.

[Mark Lam]

Created attachment 285785 [details]
proposed patch.

[Geoffrey Garen]

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

[Geoffrey Garen]

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.

[Mark Lam]

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.

[Mark Lam]

Landed in r204387: <http://trac.webkit.org/r204387>.

[Mark Lam]

(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>.