Closed Bug 1485216 Opened Last year Closed 11 months ago

CooperativeThreadPool, Scheduler and LabeledEventQueue are unused

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: marco, Assigned: froydnj)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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?
Flags: needinfo?(nfroyd)
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...
Flags: needinfo?(nfroyd)
I wonder if the related classes might be removed (with more effort): Scheduler, LabeledEventQueue, PrioritizedEventQueue, nsILabelableRunnable, etc.
Scheduler and LabeledEventQueue can be removed. PrioritezedEventQueue is in use, nsILabelableRunnable as well.
Are you planning on taking this on?
Flags: needinfo?(afarre)
Summary: CooperativeThreadPool seems unused → CooperativeThreadPool, Scheduler and LabeledEventQueue are unused
I would like to take it on, yes.
Flags: needinfo?(afarre)
Blocks: 1254240
We do use LabeledEventQueue, but shouldn't, since it shows up in performance profiles.
This compiles on my machine, at least.  We'll see if it gets through try.
Attachment #9037678 - Flags: review?(continuation)
Attachment #9037678 - Flags: review?(continuation) → review+

(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.

Flags: needinfo?(jlaster)
Flags: needinfo?(jlaster) → needinfo?(poirot.alex)
Flags: needinfo?(bhackett1024)

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...

(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!

Flags: needinfo?(poirot.alex)
Flags: needinfo?(bhackett1024)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cde93070b0
remove Scheduler and related code from xpcom/threads; r=mccr8
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee: nobody → nfroyd
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/784f80261f91
follow-up - really delete now-dead code; r=me

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

You need to log in before you can comment on or make changes to this bug.