Bug 160716

Summary: Disallow synchronous sweeping for eden GCs.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: 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 Flags
proposed patch.
fpizlo: review-
proposed patch. ggaren: review+

Description Mark Lam 2016-08-09 16:19:01 PDT
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.
Comment 1 Mark Lam 2016-08-09 17:53:40 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().
Comment 2 Mark Lam 2016-08-09 18:14:17 PDT
Created attachment 285704 [details]
proposed patch.
Comment 3 Filip Pizlo 2016-08-10 14:59:26 PDT
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.
Comment 4 Mark Lam 2016-08-10 15:03:43 PDT
Going back to disallowing collectAndSweep for EdenCollections.
Comment 5 Mark Lam 2016-08-10 16:52:02 PDT
Created attachment 285785 [details]
proposed patch.
Comment 6 Geoffrey Garen 2016-08-11 11:46:17 PDT
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 7 Geoffrey Garen 2016-08-11 13:27:38 PDT
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 8 Mark Lam 2016-08-11 13:32:47 PDT
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.
Comment 9 Mark Lam 2016-08-11 14:20:28 PDT
Landed in r204387: <http://trac.webkit.org/r204387>.
Comment 10 Mark Lam 2016-08-11 18:05:46 PDT
(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>.