Bug 167234 - We should flash a safepoint before each DFG/FTL phase
Summary: We should flash a safepoint before each DFG/FTL phase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-20 00:13 PST by Saam Barati
Modified: 2017-01-20 10:37 PST (History)
13 users (show)

See Also:


Attachments
CLI JSC Benchmarks (87.28 KB, text/plain)
2017-01-20 02:35 PST, Saam Barati
no flags Details
Indicative runs of Kraken in browser (2.19 KB, text/plain)
2017-01-20 02:46 PST, Saam Barati
no flags Details
patch (14.83 KB, patch)
2017-01-20 03:15 PST, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-01-20 00:13:33 PST
The GC thread may wait for all compilations to finish, and some compilations can take a really long time. Making the collector wait for ~20ms for large functions is not desirable. We should introduce more opportunities for the collector to proceed while it's waiting on a compiler thread. It seems natural to give the GC that opportunity before each phase is run.

Our recent Kraken regressions are caused by the GC waiting for the compiler to finish for ~20-30ms.
This particular function in Kraken has always taken a long time to compile, but because of recent GC work, the timing of when some GCs are scheduled just happen to occur when this one large function is being compiled.
Comment 1 Saam Barati 2017-01-20 02:35:00 PST
Created attachment 299338 [details]
CLI JSC Benchmarks
Comment 2 Saam Barati 2017-01-20 02:46:01 PST
Created attachment 299339 [details]
Indicative runs of Kraken in browser
Comment 3 Radar WebKit Bug Importer 2017-01-20 03:13:53 PST
<rdar://problem/30115838>
Comment 4 Saam Barati 2017-01-20 03:15:13 PST
Created attachment 299340 [details]
patch
Comment 5 Filip Pizlo 2017-01-20 04:45:29 PST
Comment on attachment 299340 [details]
patch

Any chance you could put the safe point in runandlog, which leads to a smaller changeset?
Comment 6 Filip Pizlo 2017-01-20 08:44:44 PST
(In reply to comment #5)
> Comment on attachment 299340 [details]
> patch
> 
> Any chance you could put the safe point in runandlog, which leads to a
> smaller changeset?

Never mind. I see what you did there.
Comment 7 WebKit Commit Bot 2017-01-20 10:12:08 PST
Comment on attachment 299340 [details]
patch

Clearing flags on attachment: 299340

Committed r210971: <http://trac.webkit.org/changeset/210971>
Comment 8 WebKit Commit Bot 2017-01-20 10:12:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Geoffrey Garen 2017-01-20 10:36:18 PST
Comment on attachment 299340 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGPlan.cpp:260
> +#define RUN_PHASE(phase)                                         \

This could be a function template or a function that accepts an std::function or a function that accepts a ScopedLambda.
Comment 10 Filip Pizlo 2017-01-20 10:37:39 PST
(In reply to comment #9)
> Comment on attachment 299340 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299340&action=review
> 
> > Source/JavaScriptCore/dfg/DFGPlan.cpp:260
> > +#define RUN_PHASE(phase)                                         \
> 
> This could be a function template or a function that accepts an
> std::function or a function that accepts a ScopedLambda.

But it returns on the CancelPath thing.  That's why it's a macro.