Bug 205279 – Changed jsc shell timeout mechanism to leverage the VMTraps and use CPUTime.

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

[Mark Lam]

This fixes all the timeouts that occur due to CPU time starvation when running JSC tests on a debug build.

What this means is that the timeout mechanism may trigger asynchronous OSR exits.  If a test requires no OSR exits, that test should requireOption("--usePollingTraps=true") so that the VMTraps will use its polling implementation instead.

[Radar WebKit Bug Importer]

<rdar://problem/57971874>

[Mark Lam]

Created attachment 385775 [details]
proposed patch.

[Saam Barati]

Comment on attachment 385775 [details]
proposed patch.

Does this mean all our tests are now using VMTraps all the time? Wouldn't it be good to run tests without that?

Have you verified timeouts still fire?

[Mark Lam]

(In reply to Saam Barati from comment #3)
> Comment on attachment 385775 [details]
> proposed patch.
> 
> Does this mean all our tests are now using VMTraps all the time? Wouldn't it
> be good to run tests without that?

Why would it matter for the majority of tests?  The only ones where I think it might matter are for tests that are trying to verify we don't OSR exit.  For those cases, the test can add "--usePollingTraps=true" to its required options, and all will be well.  Is there another reason why we need to have traps off?

> Have you verified timeouts still fire?

Yes, I did.  I said so in the ChangeLog.

[Mark Lam]

(In reply to Mark Lam from comment #4)
> (In reply to Saam Barati from comment #3)
> > Comment on attachment 385775 [details]
> > proposed patch.
> > 
> > Does this mean all our tests are now using VMTraps all the time? Wouldn't it
> > be good to run tests without that?
> 
> Why would it matter for the majority of tests?  The only ones where I think
> it might matter are for tests that are trying to verify we don't OSR exit. 
> For those cases, the test can add "--usePollingTraps=true" to its required
> options, and all will be well.  Is there another reason why we need to have
> traps off?

Alternatively, we can default to usePollingTraps=true on jsc tests and have tests that want to test signal traps turn it off?

[Saam Barati]

Comment on attachment 385775 [details]
proposed patch.

Can we change what our timeout numbers are if we make this change? Those numbers are tuned for CPU starvation.

[Mark Lam]

(In reply to Saam Barati from comment #6)
> Comment on attachment 385775 [details]
> proposed patch.
> 
> Can we change what our timeout numbers are if we make this change? Those
> numbers are tuned for CPU starvation.

I think we should totally do that.  But we should do that in a separate patch.  

Re your earlier question about whether we want to test without VMTraps completely, do you think there is such a need?  If so, I can always add a jsc shell option to allow the use of the old wall clock time timeout for the test that needs it.  My instinct says this is rare, and we should hold off on adding this option until a need arises.  It's not difficult to add.

[Saam Barati]

Comment on attachment 385775 [details]
proposed patch.

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

r=me

Let's please also file a bug for changing our timeouts.

> Source/JavaScriptCore/jsc.cpp:2453
> +    Thread::create("jsc Timeout Thread", [=] () {
> +        sleep(remainingTime);
> +        VMInspector::forEachVM([&] (VM& vm) -> VMInspector::FunctorStatus {
> +            if (&vm != s_vm)
> +                return VMInspector::FunctorStatus::Continue;
> +            vm.notifyNeedShellTimeoutCheck();
> +            return VMInspector::FunctorStatus::Done;
> +        });
> +    });

This can be a helper shared here and in startTimeoutThreadIfNeeded

> Source/JavaScriptCore/jsc.cpp:2477
> +        sleep(timeoutDuration / 5.0);

As we talked about in person, let's remove the / 5.0 that you used for testing

[Mark Lam]

Thanks for the review.  Landed in r253581: <http://trac.webkit.org/r253581>.

[Mark Lam]

Going to roll out the patch while I investigate why the release build is failing.

[Mark Lam]

Rolled out in r253609: <http://trac.webkit.org/r253609>.

[Alexey Proskuryakov]

Looks like this caused a variety of failures:
- broke 74 tests, as EWS predicted;
- broke webkit-jsc-cloop-test;
- made release tests 2x slower.

[Mark Lam]

Created attachment 385858 [details]
patch for landing.

[Mark Lam]

Re-landed in r253613: <http://trac.webkit.org/r253613>.