1267297 - Stop using the activity callback in Gecko

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

JS_BeginRequest and JS_EndRequest plus the related infrastructure aren't fulfilling any useful role anymore.

The one thing that is triggered by requests is the activityCallback, which embeddings can set via a friendapi function. The XPCJSRuntime uses this:
https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#1353

Maybe there are one or a few pinch points in the runtime where we could trigger this instead?

Comment 1

[Bobby Holley (:bholley)]

Yes, I think we could put a similar counter on AutoEntryScript that notifies XPConnect directly to reset the watchdog.

Or if we're not confident that we always have a proper aes on the stack when running script, we could do it on AutoJSAPI. That'd feel kinda screwy though.

Boris, what do you think?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure why the activityCallback depends on requests at all.  Is there a reason it does?

Or put another way: workers are always in a request: they just enter it before entering their event loop.  What would be wrong with doing that on the main thread too?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> I'm not quite sure why the activityCallback depends on requests at all.  Is
> there a reason it does?
> 
> Or put another way: workers are always in a request: they just enter it
> before entering their event loop.  What would be wrong with doing that on
> the main thread too?

The only thing requests are used for right now is to tell the watchdog timer that we're executing script and might need the slow script dialog (otherwise we put it to bed).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  So on workers the watchdog timer is always active even when the worker is totally idle and waiting for messages?  :(

Seems to me like having some auto object that starts up the watchdog would make sense.  Workers could use it during runnable processing (though it might be that AutoEntryScript is enough for that purpose) and yes, we could put it in AutoEntryScript.  I'm not sure how confident we are that we always have AutoEntryScript, not AutoJSAPI, when we should.  Maybe we should put it in AutoJSAPI until we actually implement the "do not enter script" provisions of AutoJSAPI.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Hmm.  So on workers the watchdog timer is always active even when the worker
> is totally idle and waiting for messages?  :(

There is no watchdog on workers. It lives on XPCJSRuntime.

> Seems to me like having some auto object that starts up the watchdog would
> make sense.  Workers could use it during runnable processing

I don't see what workers have to do with this. They're allowed to run forever and don't have a slow script dialog.

> (though it
> might be that AutoEntryScript is enough for that purpose) and yes, we could
> put it in AutoEntryScript.  I'm not sure how confident we are that we always
> have AutoEntryScript, not AutoJSAPI, when we should.  Maybe we should put it
> in AutoJSAPI until we actually implement the "do not enter script"
> provisions of AutoJSAPI.

I guess that makes sense, though the downside is a lot of unnecessary watchdog wakeups. What is blocking us from implementing those provisions in the status quo?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> There is no watchdog on workers.

Hrm.  But workers have slow script interrupts... how are those handled?

> They're allowed to run forever and don't have a slow script dialog.

They have slow script interrupts to process control messages.  Like "shut down the worker".

> What is blocking us from implementing those provisions in the status quo?

Lack of someone to do the work.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Per IRC discussion, just to clarify my understanding:

1)  On workers, we just set interrupt state when a control runnable comes in.  There is no separate watchdog
    thread.
2)  On main thread, we set interrupt state from the watchdog thread.

Comment 8

[Bobby Holley (:bholley)]

And to finish off the discussion, we decided it's probably fine to stick it on AutoEntryScript, and that the rewards outweigh the potential to let a few cases fall through the cracks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We're going to need to replace js::ContextHasOutstandingRequests uses with something else, presumably.  Or just nix them in bug 1276276.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #8)
> we decided it's probably fine to stick it
> on AutoEntryScript, and that the rewards outweigh the potential to let a few
> cases fall through the cracks.

I'm Try servering a patch to trigger the activity stuff from AutoEntryScript. It's the only blocker for request API removal I think.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1267297 - Use AutoEntryScript for script activity bookkeeping instead of the request machinery. r=bholley

Comment 13

[Jan de Mooij [:jandem]]

This is green on Try and the e10s slow script notification behaves correctly (it disappears when the script finishes).

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 9003485 [details]
Bug 1267297 - Use AutoEntryScript for script activity bookkeeping instead of the request machinery. r=bholley

Bobby Holley (:bholley) has approved the revision.

Comment 15

[Jan de Mooij [:jandem]]

While working on removing JSAutoRequest I found out about mozilla::AutoSlowOperation, it needs an AutoScriptActivity too I suppose because it relied on the JSAutoRequest in AutoJSAPI. I'll update the patch.

Comment 16

[Jan de Mooij [:jandem]]

I'll land this after the merge next week just in case there will be subtle regressions.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04eb549952d0
Use AutoEntryScript for script activity bookkeeping instead of the request machinery. r=bholley

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/04eb549952d0