1485216 - CooperativeThreadPool, Scheduler and LabeledEventQueue are unused

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Marco Castelluccio [:marco]]

It was introduced in bug 1350432, but it was never used as far as I can tell.
Should we remove it, or will it ever be used?

If we plan to use it at some point and don't want it to bitrot, can we compile it in debug only so we don't pay the price for it in builds we ship to users?

Comment 1

[Nathan Froyd [:froydnj]]

Sure, we can remove it.  Kind of unfortunate, but that's life.

Looking at my Linux64 binary, there's only about a kilobyte of CooperativeThreadPool code, so it won't make a lot of difference either way...

Comment 2

[Marco Castelluccio [:marco]]

I wonder if the related classes might be removed (with more effort): Scheduler, LabeledEventQueue, PrioritizedEventQueue, nsILabelableRunnable, etc.

Comment 3

[Andreas Farre [:farre]]

Scheduler and LabeledEventQueue can be removed. PrioritezedEventQueue is in use, nsILabelableRunnable as well.

Comment 4

[Marco Castelluccio [:marco]]

Are you planning on taking this on?

Comment 5

[Andreas Farre [:farre]]

I would like to take it on, yes.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

We do use LabeledEventQueue, but shouldn't, since it shows up in performance profiles.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: remove Scheduler and related code from xpcom/threads

This compiles on my machine, at least.  We'll see if it gets through try.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #7)

Created attachment 9037678 [details] [diff] [review]
remove Scheduler and related code from xpcom/threads

This compiles on my machine, at least. We'll see if it gets through try.

Hope springs eternal, apparently:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be8813dd8951de83331e39dad5c84f22aa659eb9

Apparently this broke a lot of devtools tests. I thought the obvious changes would work, but apparently not.

jlast, do you know (or can you find someone) who can point me in the right direction? e.g. the changes in devtools/server/startup/content-process.jsm:

https://hg.mozilla.org/try/rev/be8813dd8951de83331e39dad5c84f22aa659eb9#l1.1

are AFAICT just reversing the changes from:

https://searchfox.org/mozilla-central/diff/51c04f774fdb94cc997dc46200ec821425f1236d/devtools/server/content-server.jsm#65

and similar changes elsewhere in devtools, but we wind up with strange errors in devtools tests as you can see in the above try link.

Comment 9

[Alexandre Poirot [:ochameau]]

Wouldn't you be missing one occurence in frame.js?
devtools/server/tests/mochitest/test_connectToFrame.html fails with the following exception:
[task 2019-01-19T03:23:29.510Z] 03:23:29 INFO - GECKO(5034) | DBG-SERVER: Packet 4 sent from "server1.conn0.child1/frameTarget1"
[task 2019-01-19T03:23:29.511Z] 03:23:29 INFO - GECKO(5034) | Handler function threw an exception: TypeError: Cu.unblockThreadedExecution is not a function
[task 2019-01-19T03:23:29.512Z] 03:23:29 INFO - GECKO(5034) | Stack: onDisconnect<@resource://devtools/server/startup/frame.js:107:7

I can look more at that tomorrow if that's not just that. I wouldn't be surprised if that highlights existing races in DevTools tests...

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Alexandre Poirot [:ochameau] from comment #9)

Wouldn't you be missing one occurence in frame.js?
devtools/server/tests/mochitest/test_connectToFrame.html fails with the following exception:
[task 2019-01-19T03:23:29.510Z] 03:23:29 INFO - GECKO(5034) | DBG-SERVER: Packet 4 sent from "server1.conn0.child1/frameTarget1"
[task 2019-01-19T03:23:29.511Z] 03:23:29 INFO - GECKO(5034) | Handler function threw an exception: TypeError: Cu.unblockThreadedExecution is not a function
[task 2019-01-19T03:23:29.512Z] 03:23:29 INFO - GECKO(5034) | Stack: onDisconnect<@resource://devtools/server/startup/frame.js:107:7

I can look more at that tomorrow if that's not just that. I wouldn't be surprised if that highlights existing races in DevTools tests...

Gah, thank you. I feel kind of dumb now. Thank you for looking!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cde93070b0
remove Scheduler and related code from xpcom/threads; r=mccr8

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/d9cde93070b0

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/784f80261f91
follow-up - really delete now-dead code; r=me

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/784f80261f91

Comment 15

[Andrew Creskey [:acreskey]]

The LabeledEventQueues, which were composed of multiple queues, may have helped in getting an earlier TTFI,
see my comment 13 here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1522827