Bug 167234 – We should flash a safepoint before each DFG/FTL phase

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 299338 [details]
CLI JSC Benchmarks

[Saam Barati]

Created attachment 299339 [details]
Indicative runs of Kraken in browser

[Radar WebKit Bug Importer]

<rdar://problem/30115838>

[Saam Barati]

Created attachment 299340 [details]
patch

[Filip Pizlo]

Comment on attachment 299340 [details]
patch

Any chance you could put the safe point in runandlog, which leads to a smaller changeset?

[Filip Pizlo]

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

[WebKit Commit Bot]

Comment on attachment 299340 [details]
patch

Clearing flags on attachment: 299340

Committed r210971: <http://trac.webkit.org/changeset/210971>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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.

[Filip Pizlo]

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