Closed Bug 1267297 Opened 8 years ago Closed 6 years ago

Stop using the activity callback in Gecko

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: till, Assigned: jandem)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley) → needinfo?(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?
Flags: needinfo?(bzbarsky)
(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).
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Blocks: 722345
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Remove request-related functions, fields, and JSAPI members → Gecko should always be in a request
We're going to need to replace js::ContextHasOutstandingRequests uses with something else, presumably.  Or just nix them in bug 1276276.
Depends on: 1276276
(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.
Summary: Gecko should always be in a request → Stop using the activity callback in Gecko
This is green on Try and the e10s slow script notification behaves correctly (it disappears when the script finishes).
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.
Attachment #9003485 - Flags: review+
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.
I'll land this after the merge next week just in case there will be subtle regressions.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/04eb549952d0
Use AutoEntryScript for script activity bookkeeping instead of the request machinery. r=bholley
Assignee: nobody → jdemooij
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/04eb549952d0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: