Parallel compilation fix: the optimizer thread shouldn't change the allow allocation state on Asser…

Parallel compilation fix: the optimizer thread shouldn't change the allow allocation state on AssertNoAllocation scopes.

BUG=
R=mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14551

[mvstanton, 2013-05-06 09:48:57]

Could you have a quick look? Thanks!
--Michael

[Sven Panne, 2013-05-06 10:02:48]

Why is this CL necessary?

[Yang, 2013-05-06 10:14:32]

On 2013/05/06 10:02:48, Sven Panne wrote:
> Why is this CL necessary?

@Sven: the optimization thread could use this scope concurrently to the main
thread and screw up the allocation state for the main thread. For example, the
main thread is entering the scope, forbidding allocation. Shortly after, the
optimizer thread is exiting the scope, allowing allocation for both threads.
Drive-by LGTM.
Maybe add a TODO(yangguo) for me to make it work for both threads similarly for
the HandleDereferenceGuard.

[Sven Panne, 2013-05-06 10:17:05]

On 2013/05/06 10:14:32, Yang wrote:
> On 2013/05/06 10:02:48, Sven Panne wrote:
> > Why is this CL necessary?
> 
> @Sven: the optimization thread could use this scope concurrently to the main
> thread and screw up the allocation state for the main thread. [...]

o_O Where are the memory barriers for this? This really scares the hell out of
me...

[Yang, 2013-05-06 10:34:45]

On 2013/05/06 10:17:05, Sven Panne wrote:
> On 2013/05/06 10:14:32, Yang wrote:
> > On 2013/05/06 10:02:48, Sven Panne wrote:
> > > Why is this CL necessary?
> > 
> > @Sven: the optimization thread could use this scope concurrently to the main
> > thread and screw up the allocation state for the main thread. [...]
> 
> o_O Where are the memory barriers for this? This really scares the hell out of
> me...

The problem is that those scopes only work with a single stack. Currently we
make those scopes a no-op for the optimizer thread, so no memory barrier is
necessary, but this was messed up for exiting the scope, which this CL fixes.

[Michael Starzinger, 2013-05-06 10:48:20]

LGTM. I bet there are a million other places where the compiler thread mutates
the isolate. :(

[Sven Panne, 2013-05-06 10:56:46]

On 2013/05/06 10:48:20, Michael Starzinger wrote:
> LGTM. I bet there are a million other places where the compiler thread mutates
> the isolate. :(

That's what I fear, too. In general mutating *any* structure owned by the
Isolate in the compiler thread leads to disaster. The only mechanical way of
finding all those places is probably a 3-step process:

   * Remove all "train-wrecks" isolate->foo()->bar()->baz()

   * Make the newly created Isolate methods (and all old ones) const-correct.

   * Make sure the compiler thread only calls const methods.

 :-/

[mvstanton, 2013-05-06 10:58:58]

On 2013/05/06 10:48:20, Michael Starzinger wrote:
> LGTM. I bet there are a million other places where the compiler thread mutates
> the isolate. :(

Thanks for the look. I added the TODO for Yang.

[mvstanton, 2013-05-06 10:59:23]

Committed patchset #2 manually as r14551 (presubmit successful).