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+

Filip Pizlo
Reported 2016-11-01 12:05:33 PDT
Patch forthcoming.
Attachments
possible patch (25.90 KB, patch)
2016-11-16 22:17 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (38.21 KB, patch)
2016-11-18 12:31 PST, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 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.
Filip Pizlo
Comment 2 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.
Filip Pizlo
Comment 3 2016-11-18 12:31:14 PST
Created attachment 295178 [details] the patch
Geoffrey Garen
Comment 4 2016-11-18 12:59:49 PST
Comment on attachment 295178 [details] the patch r=me
Oliver Hunt
Comment 5 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?
Filip Pizlo
Comment 6 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.
Filip Pizlo
Comment 7 2016-11-18 14:12:15 PST
Landed in http://trac.webkit.org/changeset/208897
Note You need to log in before you can comment on or make changes to this bug.