Bug 187196 - Don't use tracePoints in JS/Wasm entry
Summary: Don't use tracePoints in JS/Wasm entry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-29 14:50 PDT by Saam Barati
Modified: 2018-07-05 12:56 PDT (History)
14 users (show)

See Also:


Attachments
patch (5.63 KB, patch)
2018-06-29 15:51 PDT, Saam Barati
mark.lam: review-
Details | Formatted Diff | Diff
patch (4.45 KB, patch)
2018-06-29 15:54 PDT, 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 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.