Open Bug 1435689 Opened 2 years ago Updated 2 years ago

Remove cooperative scheduler

Categories

(Core :: DOM: Content Processes, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: jandem, Unassigned)

References

(Blocks 1 open bug)

Details

We would like to remove cooperative scheduling from SpiderMonkey, since we're not going to use it for Quantum DOM.

There are a bunch of calls to JS_NewCooperativeContext, JS_ResumeCooperativeContext, JS_YieldCooperativeContext, etc that we'd have to remove from Gecko first.
Nathan, I see you and mccr8 reviewed a lot of this code. Is this something you would be interested in cleaning up? :)
Flags: needinfo?(nfroyd)
I know we have decided not to move forward on qdom at the moment, but given blink is trying to implement cooperative scheduling should we perhaps wait before nuking the code?

https://groups.google.com/a/chromium.org/d/msg/blink-dev/SRYCUwSwA2A/Eck0P55XBAAJ

I mean, if we decide we need to implement something in the future, how hard is it to bring this stuff back vs maintaining it for a while?
Ben's comment 2 is basically what I would have asked.

The cooperative scheduling stuff is pretty invasive, but if it's not actively blocking things, I'd like to try and hold on to it for a little longer.  Or is the motivation here that SpiderMonkey wants to make some changes, and those changes would require thinking Very Hard about how cooperative scheduling works, and trying to insert the relevant calls in places would be difficult without some sort of test coverage of cooperative scheduling?
Flags: needinfo?(nfroyd)
(Ahead of this and bug 1434030, we asked Ehsan who asked Andreas who confirmed plans against qdom and gave the go-ahead to remove support.)

Given that the situation here won't be changing in any short amount of time, it seems like choosing to hold off means keeping this non-trivial dead functionality around for at least a year or two.  Given that we don't have any way to test the cooperative functionality in the browser and the shell testing machinery lacks expressiveness to really test it either and how easy it is to regress, this seems like a recipe for bitrot and mental overhead.

Also, to wit, from the description in that chromium post, it looks like they aren't using our cooperative-multi-thread approach, but are instead using event loop nesting.  It seems like either that will work and we could do likewise (without JS engine cooperative JSContext support) or it won't, and if they switch to doing something like what we did, it'll take a while.
(In reply to Luke Wagner [:luke] from comment #4)
> (Ahead of this and bug 1434030, we asked Ehsan who asked Andreas who
> confirmed plans against qdom and gave the go-ahead to remove support.)

For avoidance of doubt: this means that even attempting to turn on cooperative scheduling in current builds will already lead to bustage, independent of any bitrot we might have had before?
You mean specifically bug 1434030?  Yes.  (That's why I asked first.)  It's an example of some of the runtime overhead: for lack of a fixed thread, a triple-dependent load was needed in prologues to check the stack limit.
OK, thanks for the confirmation.  So we have:

1. The person who's most likely to work on this (Andreas) says to remove support.
2. Ehsan trusts Andreas to say yay or nay on this, which is worth a lot.
3. Things are already broken with JIT changes.

I'd say we're good to go on ripping things out, then.  Happy to review patches when they get written and/or try to contribute some myself.  Sound good?
Flags: needinfo?(jdemooij)
Also: It's pretty unfortunate that we spent a lot of time getting this code into the tree + various kinds of support and then we're going to begin ripping it out shortly thereafter (and probably bits and pieces will still be coming out for the rest of this year, at least).  Do we think the design is flawed, we didn't push hard enough on making it happen (perf measurements, flushing bugs, whatever), something else, or some combination of the above?
FWIW chrome's measured benefits are specifically for problematic 3rd party iframes that they can't oop on mobile.  It's possible we would see similar gains on that more focused use case vs a generic measurement against all windows in a process.
(In reply to Nathan Froyd [:froydnj] from comment #7)
> I'd say we're good to go on ripping things out, then.  Happy to review
> patches when they get written and/or try to contribute some myself.  Sound
> good?

Sounds good to me! Thanks.
Flags: needinfo?(jdemooij)
Priority: -- → P3
FWIW, I have some stuff on the JS side that's blocked on this, so I think I'll start landing patches that (for now) MOZ_CRASH in the relevant APIs. That way I'm not blocked on Gecko code removal and at the same time nobody can start using this.
The JS side of this is done now (the remaining APIs will MOZ_CRASH), so we can probably remove a lot of code on the Gecko side too.
You need to log in before you can comment on or make changes to this bug.