Don't use tracePoints in JS/Wasm entry

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

Created attachment 343958 [details]
patch

[Saam Barati]

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.

[Mark Lam]

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. :)

[Saam Barati]

Created attachment 343959 [details]
patch

[Simon Fraser (smfr)]

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.

[Saam Barati]

(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();
```

[WebKit Commit Bot]

Comment on attachment 343959 [details]
patch

Clearing flags on attachment: 343959

Committed r233378: <https://trac.webkit.org/changeset/233378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41659565>

[Geoffrey Garen]

Another solution is to work with the team responsible for trace points and make them not super slow.

[Saam Barati]

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

[Filip Pizlo]

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