Bug 164282 - Concurrent GC should be able to run splay in debug mode and earley/raytrace in release mode with no perf regression
Summary: Concurrent GC should be able to run splay in debug mode and earley/raytrace i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 149432
  Show dependency treegraph
 
Reported: 2016-11-01 12:05 PDT by Filip Pizlo
Modified: 2016-11-18 14:12 PST (History)
8 users (show)

See Also:


Attachments
possible patch (25.90 KB, patch)
2016-11-16 22:17 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (38.21 KB, patch)
2016-11-18 12:31 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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