Make js::HelperThread::thread a js::Thread instead of a PRThread

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

This ended up boiling more of the ocean than I'd hoped for, but the resulting
code is much nicer.
Created attachment 8775790 [details] [diff] [review]
Make js::HelperThread::thread a js::Thread instead of a PRThread
Attachment #8775790 - Flags: review?(terrence)
Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
For some reason `bool js::Thread::init<void (&)(JSRuntime*), JSRuntime*&>(void (&&&)(JSRuntime*), JSRuntime*&&&)` is calling vanilla operator new.

> 000000000068c560 <__ZN2js6Thread4initIRFvP9JSRuntimeEJRS3_EEEbOT_DpOT0_>:
>   <snip>
>   68c5d7:	e8 b6 cc a0 00       	callq  1099292 <__Znwm$stub>
>   <snip>
Inside `js::Thread::init`:

>     auto trampoline = new Trampoline(mozilla::Forward<F>(f),
>                                      mozilla::Forward<Args>(args)...);

Oh.

That should be js_new.
Waldo says that this is on purpose so that we can lift js::Thread to mfbt/mozglue.

We should either

(1) fix check-vanilla-allocations to ignore js::Thread::init
(2) make js::Thread parameterized by an alloc policy, similar to mozilla::Vector

I don't have strong opinions either way.
Terrence, happen to have an opinion on comment 5?

I'm leaning towards (2) because I think it will be easier (the check-vanilla-allocations script doesn't have any understanding of who is calling who, only what symbols end up in the object file) and is also more correct, in that we can continue avoiding use of the system allocator like the rest of js/ does and be consistent.
Err, forgot to ni -- see comment 3 through comment 6.
Flags: needinfo?(terrence)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #5)
> Waldo says that this is on purpose so that we can lift js::Thread to
> mfbt/mozglue.
> 
> We should either
> 
> (1) fix check-vanilla-allocations to ignore js::Thread::init
> (2) make js::Thread parameterized by an alloc policy, similar to
> mozilla::Vector
> 
> I don't have strong opinions either way.

Or (3) we could make it js_new for now and whoever moves it to mfbt can change that line?
Flags: needinfo?(terrence)
Comment on attachment 8775790 [details] [diff] [review]
Make js::HelperThread::thread a js::Thread instead of a PRThread

Review of attachment 8775790 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/HelperThreads.cpp
@@ +152,5 @@
>          }
>      }
>  
>      /* Wait for in progress entries to finish up. */
> +    for (HelperThread& helper : *HelperThreadState().threads) {

Let's use auto& here to be consistent with the other iterations.
Attachment #8775790 - Flags: review?(terrence) → review+

Comment 11

2 years ago
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c1777276fe
Make js::HelperThread::thread a js::Thread instead of a PRThread; r=terrence

Comment 12

2 years ago
backoutbugherderlanding
This or something else from the push appears to have turned 10.10 debug cpp tests permared: https://treeherder.mozilla.org/logviewer.html#?job_id=33028238&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a40cee73f3
Flags: needinfo?(nfitzgerald)
Created attachment 8776727 [details] [diff] [review]
Make js::HelperThread::thread a js::Thread instead of a PRThread

It ended up being this patch that caused that failure. We were doing a
`MOZ_RELEASE_ASSERT` on the allocation of the thread's trampoline but that
doesn't play well with `js_new`. I just made it an `AutoOOMUnsafeRegion` to keep
the old semantics, even though it seems like we could maybe get away with
handling the failure.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=710c4a0ee5ed
Attachment #8776727 - Flags: review+
Attachment #8775790 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)

Comment 14

2 years ago
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60cd2460476f
Make js::HelperThread::thread a js::Thread instead of a PRThread; r=terrence

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60cd2460476f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1291349
You need to log in before you can comment on or make changes to this bug.