Bug 164282 – Concurrent GC should be able to run splay in debug mode and earley/raytrace in release mode with no perf regression

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 295178 [details]
the patch

[Geoffrey Garen]

Comment on attachment 295178 [details]
the patch

r=me

[Oliver Hunt]

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?

[Filip Pizlo]

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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/208897