Bug 187196

Summary: Don't use tracePoints in JS/Wasm entry
Product: WebKit Reporter: Saam Barati <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, rmorisset, simon.fraser, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
mark.lam: review-
patch none

Description Saam Barati 2018-06-29 14:50:15 PDT
trace points are expensive when run many times. Entering the VM can happen many times and in arbitrary, user controlled, ways. Entering wasm can also happen in arbitrary, user controlled ways, therefore, these should not use trace points here unconditionally.
Comment 1 Saam Barati 2018-06-29 15:51:45 PDT
Created attachment 343958 [details]
patch
Comment 2 Saam Barati 2018-06-29 15:52:10 PDT
Comment on attachment 343958 [details]
patch

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

> Source/WebCore/workers/WorkerThread.cpp:146
> +        Thread::setCurrentThreadIsUserInteractive();

need to revert this.
Comment 3 Mark Lam 2018-06-29 15:54:54 PDT
Comment on attachment 343958 [details]
patch

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

r=me with fix.

>> Source/WebCore/workers/WorkerThread.cpp:146
>> +        Thread::setCurrentThreadIsUserInteractive();
> 
> need to revert this.

I was just about to ask. :)
Comment 4 Saam Barati 2018-06-29 15:54:54 PDT
Created attachment 343959 [details]
patch
Comment 5 Simon Fraser (smfr) 2018-06-29 16:15:11 PDT
Comment on attachment 343959 [details]
patch

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

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:61
> +        if (Options::useTracePoints())
> +            tracePoint(VMEntryScopeStart);

I don't like this change. It's very useful to find times when JS is running in traces, and this entry point hasn't proven to be too frequent in the past.
Comment 6 Saam Barati 2018-06-29 16:32:49 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 343959 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343959&action=review
> 
> > Source/JavaScriptCore/runtime/VMEntryScope.cpp:61
> > +        if (Options::useTracePoints())
> > +            tracePoint(VMEntryScopeStart);
> 
> I don't like this change. It's very useful to find times when JS is running
> in traces, and this entry point hasn't proven to be too frequent in the past.

I agree this is very useful. But it being useful is not worth making top level VMEntryScope ~4x slower. We need to:
- Get in the habit of setting the JSC_useTracePoints environment variable to 1 before we use our tracing tools.
- Get other tools we use to set this environment variable too

Programs like this become 4x slower:
```
function foo() {
    let start = Date.now();
    for (let i = 0; i < 10000; ++i) {
        setTimeout(function(){}, 0)
    }
    setTimeout(function(){console.log(Date.now() - start)}, 0);
}
foo();
```
and
```
async function foo() {
    let start = Date.now();
    for (let i = 0; i < 100000; ++i)
        await i;
    console.log(Date.now() - start);
}
foo();
```
Comment 7 WebKit Commit Bot 2018-06-29 17:05:42 PDT
Comment on attachment 343959 [details]
patch

Clearing flags on attachment: 343959

Committed r233378: <https://trac.webkit.org/changeset/233378>
Comment 8 WebKit Commit Bot 2018-06-29 17:05:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-06-29 17:06:31 PDT
<rdar://problem/41659565>
Comment 10 Geoffrey Garen 2018-07-05 12:41:56 PDT
Another solution is to work with the team responsible for trace points and make them not super slow.
Comment 11 Saam Barati 2018-07-05 12:44:55 PDT
(In reply to Geoffrey Garen from comment #10)
> Another solution is to work with the team responsible for trace points and
> make them not super slow.

I'm already doing that. The plan is to revert this if trace points become fast.
Comment 12 Filip Pizlo 2018-07-05 12:56:14 PDT
(In reply to Saam Barati from comment #11)
> (In reply to Geoffrey Garen from comment #10)
> > Another solution is to work with the team responsible for trace points and
> > make them not super slow.
> 
> I'm already doing that. The plan is to revert this if trace points become
> fast.

I don't think we should ever revert this change.  We should aim for VM entry to be about as fast as a normal JS->JS or wasm->wasm procedure call.

We wouldn't call system API in prologues/epilogues of functions, so we shouldn't call system API in VM entry.  I think that remains true even if that API becomes very fast.  Even super fast APIs never get fast enough for use in prologues and epilogues.