Bug 164282

Summary: Concurrent GC should be able to run splay in debug mode and earley/raytrace in release mode with no perf regression
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, ggaren, keith_miller, mark.lam, msaboff, rniwa, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 149432    
Attachments:
Description Flags
possible patch
none
the patch ggaren: review+

Description Filip Pizlo 2016-11-01 12:05:33 PDT 
Patch forthcoming.
Comment 1 Filip Pizlo 2016-11-01 12:07:43 PDT 
Here's a sequence of events.  I will mark events that happen with the world stopped with an *.

(0) GC is running, mutator has heap access
(1) op_enter in codeBlock
(2) writeBarrier(codeBlock)
(3) codeBlock.visitChildren
(4) valueProfile(codeBlock)
(5) codeBlock no longer on the stack
(6*) scan stack
(7*) end marking
(8*) run all unconditional finalizers, including codeBlock's.

In this world, the value profile update will not be handled by the GC because that's done in visitChildren and visitChildren will not run after the value profiling.

But we already have finalizers that can do this, and the ValueProfile work is finalization (it clears references to things) not marking (it never marks things).

So, to fix this bug, we just need to move the ValueProfile logic into the UnconditionalFinalizer.
Comment 2 Filip Pizlo 2016-11-16 22:17:48 PST 
Created attachment 295029 [details]
possible patch

This also fixes a bug that caused very bad performance on earley and raytrace.

I'm still getting the hang of scheduling a retreating wavefront collector.
Comment 3 Filip Pizlo 2016-11-18 12:31:14 PST 
Created attachment 295178 [details]
the patch
Comment 4 Geoffrey Garen 2016-11-18 12:59:49 PST 
Comment on attachment 295178 [details]
the patch

r=me
Comment 5 Oliver Hunt 2016-11-18 13:01:13 PST 
Comment on attachment 295178 [details]
the patch

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

r=me

> PerformanceTests/JetStream/cdjs/benchmark.js:49
> +        print(result.time);

debugging comment?
Comment 6 Filip Pizlo 2016-11-18 13:53:02 PST 
(In reply to comment #5)
> Comment on attachment 295178 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295178&action=review
> 
> r=me
> 
> > PerformanceTests/JetStream/cdjs/benchmark.js:49
> > +        print(result.time);
> 
> debugging comment?

Good catch!  Removed.
Comment 7 Filip Pizlo 2016-11-18 14:12:15 PST 
Landed in http://trac.webkit.org/changeset/208897