Closed Bug 1290287 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 1 obsolete file)

This ended up boiling more of the ocean than I'd hoped for, but the resulting
code is much nicer.
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+
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/60cd2460476f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1291349
You need to log in before you can comment on or make changes to this bug.