Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Description • 8 years ago

This ended up boiling more of the ocean than I'd hoped for, but the resulting
code is much nicer.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 2 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7152cba5644c

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 3 • 8 years ago

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>

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 4 • 8 years ago

Inside `js::Thread::init`:

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

Oh.

That should be js_new.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 5 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 6 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 7 • 8 years ago

Err, forgot to ni -- see comment 3 through comment 6.

	Terrence Cole [:terrence]
	Comment 8 • 8 years ago

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

	Terrence Cole [:terrence]
	Comment 9 • 8 years ago

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.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 10 • 8 years ago

Try push with review fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28d57ab7197c&group_state=expanded

	Pulsebot
	Comment 11 • 8 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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 8 years ago
backout bugherder landing

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

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] Assignee
	Comment 13 • 8 years ago

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

	Pulsebot
	Comment 14 • 8 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

	Carsten Book [:Tomcat]
	Comment 15 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/60cd2460476f