[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST operations.

[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org

Committed: https://crrev.com/09e921d4c8ad734c1a6ade96228a60886ea5f19f
Cr-Commit-Position: refs/heads/master@{#38526}

[rmcilroy, 2016-08-05 16:00:55]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 16:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-08-05 16:17:12]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-08-05 16:17:16]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 16:17:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 16:40:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-05 16:40:15]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-08-05 17:06:38]

rmcilroy@chromium.org changed reviewers:
+ adamk@chromium.org

[rmcilroy, 2016-08-05 17:06:39]

Adam, could you take a look please? Thanks.

[adamk, 2016-08-05 18:38:09]

adamk@chromium.org changed reviewers:
+ marja@chromium.org

[adamk, 2016-08-05 18:38:10]

This looks fine to me, but I think marja is probably a better reviewer for the
internals of this AstValue stuff.

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h#newcode819
src/globals.h:819: kHandleDereferenceAllowed,
These can just be kAllowed and kDisallowed, since you're using an enum class.

[marja, 2016-08-08 08:20:13]

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.cc (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.cc:272: 
This code snippet seems to be on a wrong abstraction level; AstValueFactory
shouldn't know anything about Ignition. Can this be done somewhere on the
caller's side a couple of layers up?

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast.h#newcode321
src/ast/ast.h:321: bool IsPropertyName(
For all calls, the guarantee you want to give is that the call w/ kAllowed
yields the same result as the call w/ kDisallowed + CanonicalHandleScope, right?
Can this be documented somewhere?

https://codereview.chromium.org/2223523002/diff/20001/src/ast/variables.h
File src/ast/variables.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/variables.h#new...
src/ast/variables.h:93: name_is_identical_to(isolate->factory()->this_string(),
deref_mode);
If handle dereferencing is not allowed (i.e., we are on a background thread), it
looks scary that we do "isolate->factory()->this_string()". The conservative
approach would be to not access Isolate / Heap at all. (That's what the
streaming parser does.) Does Ignition take a different approach, with some
reasoning that reading data from Isolate / Heap is ok because <something>?

Also, it's nontrivial to see what guarantees that
isolate->factory()->this_string() is affected by the CanonicalHandleScope. (Just
reading the comments above CanonicalHandleScope... I don't know the
implementation details..) It says "It changes the existing HandleScope so that
Handles created within are canonicalized.". But is this_string() in the previous
HandleScope? Who guarantees that there are no HandleScopes inbetween (and there
will never be)? What does this mean for global handles?

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h#newcode819
src/globals.h:819: kHandleDereferenceAllowed,
On 2016/08/05 18:38:10, adamk wrote:
> These can just be kAllowed and kDisallowed, since you're using an enum class.

+1

[rmcilroy, 2016-08-08 13:05:42]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 13:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2016-08-08 13:05:56]

Thanks for the comments, PTAL.

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.cc (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.cc:272: 
On 2016/08/08 08:20:13, marja wrote:
> This code snippet seems to be on a wrong abstraction level; AstValueFactory
> shouldn't know anything about Ignition. Can this be done somewhere on the
> caller's side a couple of layers up?

Moved up to api.cc and compiler-dispatcher-job.cc.

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/ast.h#newcode321
src/ast/ast.h:321: bool IsPropertyName(
On 2016/08/08 08:20:13, marja wrote:
> For all calls, the guarantee you want to give is that the call w/ kAllowed
> yields the same result as the call w/ kDisallowed + CanonicalHandleScope,
right?
> Can this be documented somewhere?

For GetCallType yes this is the case. For IsPropertyName both should actually
give the same result (even without CanonicalHandleScope), it's just that
kDisallowed might be slower since it uses StringToArrayIndex() instead of
String::ToArrayIndex().

Added comments as such.

https://codereview.chromium.org/2223523002/diff/20001/src/ast/variables.h
File src/ast/variables.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/ast/variables.h#new...
src/ast/variables.h:93: name_is_identical_to(isolate->factory()->this_string(),
deref_mode);
On 2016/08/08 08:20:13, marja wrote:
> If handle dereferencing is not allowed (i.e., we are on a background thread),
it
> looks scary that we do "isolate->factory()->this_string()". The conservative
> approach would be to not access Isolate / Heap at all. (That's what the
> streaming parser does.) Does Ignition take a different approach, with some
> reasoning that reading data from Isolate / Heap is ok because <something>?

I believe it is OK to access constant heap roots off the main thread because
these will never change (they are in the immortal immovable heap page and are
constant, so the object pointer will never change. There is special casing in
HandleBase::IsDereferenceAllowed for these types of values for this reason
(otherwise this CL would crash here since it is called from within a
DisallowHandleDereferenceScope(). 


> Also, it's nontrivial to see what guarantees that
> isolate->factory()->this_string() is affected by the CanonicalHandleScope.
(Just
> reading the comments above CanonicalHandleScope... I don't know the
> implementation details..) It says "It changes the existing HandleScope so that
> Handles created within are canonicalized.". But is this_string() in the
previous
> HandleScope? Who guarantees that there are no HandleScopes inbetween (and
there
> will never be)? What does this mean for global handles?

Yes sorry that comment didn't make this too clear. The CanonicalHandleScope uses
the root handle for any handles pointing to valid roots created within the
scope, so this guarantees that any Handles which point to
isolate->factory()->this_string() will all have the same root handle location. 

Updated the comments to clarify these points.

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/2223523002/diff/20001/src/globals.h#newcode819
src/globals.h:819: kHandleDereferenceAllowed,
On 2016/08/08 08:20:13, marja wrote:
> On 2016/08/05 18:38:10, adamk wrote:
> > These can just be kAllowed and kDisallowed, since you're using an enum
class.
> 
> +1

Good point! :). Done.

[commit-bot: I haz the power, 2016-08-08 13:06:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 13:06:55]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6543)

[rmcilroy, 2016-08-08 13:23:31]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 13:23:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 13:53:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 13:53:39]

Dry run: This issue passed the CQ dry run.

[marja, 2016-08-09 08:33:26]

lgtm

Some offline discussion:

This CL gives the same background thread <-> Isolate / Heap interaction
guarantee than the optimizing concurrent compiler.

It's probably just not clear / well documented what the expected background
thread <-> Isolate / Heap interaction guarantee is, but that's not a problem of
this CL.

[rmcilroy, 2016-08-10 08:53:30]

On 2016/08/09 08:33:26, marja wrote:
> lgtm
> 
> Some offline discussion:
> 
> This CL gives the same background thread <-> Isolate / Heap interaction
> guarantee than the optimizing concurrent compiler.
> 
> It's probably just not clear / well documented what the expected background
> thread <-> Isolate / Heap interaction guarantee is, but that's not a problem
of
> this CL.

I'll land this as-is now and think about whether there is an approach to better
segregate background thread <-> Isolate / Heap interaction going forward.

[rmcilroy, 2016-08-10 08:53:40]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-08-10 08:53:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 09:03:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 09:03:33]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21491)

[rmcilroy, 2016-08-10 09:29:15]

I still need an lg from an AST owner. TBRing Adam since he thought it looked
fine. Marja, shouldn't you also be an owner of ast? :).

[rmcilroy, 2016-08-10 09:29:31]

Description was changed from

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
==========

to

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org
==========

[rmcilroy, 2016-08-10 09:29:39]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-08-10 09:29:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 09:33:05]

Description was changed from

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org
==========

to

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org
==========

[commit-bot: I haz the power, 2016-08-10 09:33:06]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-10 09:33:25]

Description was changed from

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org
==========

to

==========
[Interpreter] Avoid dereferencing handles on BytecodeGenerator for AST
operations.

Updates a number of AST operations to avoid dereferencing handles
such that they can safely be called off-thread. Also adds a
HandleDereferenceMode argument to some operations where handles are
compared. If handle dereferencing is allowed, the handles are compared
directly, if not then their locations are compared (which relies on the
handles being created in a CanonicalHandleScope).

BUG=v8:5203
TBR=adamk@chromium.org

Committed: https://crrev.com/09e921d4c8ad734c1a6ade96228a60886ea5f19f
Cr-Commit-Position: refs/heads/master@{#38526}
==========

[commit-bot: I haz the power, 2016-08-10 09:33:26]

Patchset 3 (id:??) landed as
https://crrev.com/09e921d4c8ad734c1a6ade96228a60886ea5f19f
Cr-Commit-Position: refs/heads/master@{#38526}