Add faster, but unsafe version of LoadInternalField.

Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
Committed: https://crrev.com/d073777bbf60df574ad0ba4b6069c90752d01ff1
Cr-Commit-Position: refs/heads/master@{#38535}

[Alfonso, 2016-07-26 14:29:46]

Description was changed from

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
==========

to

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
==========

[Alfonso, 2016-07-26 14:29:46]

peterssen@google.com changed reviewers:
+ vogelheim@chromium.org

[Alfonso, 2016-07-26 14:29:57]

On 2016/07/26 14:29:46, Alfonso wrote:
> mailto:peterssen@google.com changed reviewers:
> + mailto:vogelheim@chromium.org

PTAL.

[vogelheim, 2016-07-27 09:50:56]

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler.cc
File src/fast-accessor-assembler.cc (right):

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler...
src/fast-accessor-assembler.cc:94: FastAccessorAssembler::ValueId
FastAccessorAssembler::LoadInternalFieldUnsafe(
I briefly discussed handling the LoadInternalField[Unsafe] variants. How about
we change it like this:

1, we'll have both versions, LoadInternalFieldChecked and
LoadInternalFieldUnsafe.

2, There's a FLAG_debug_code, that we use elsewhere to instruct the compiler to
emit additional runtime checks (and crash if they fail). If this is set, we'll
still check the assumptions in the *Unsafe case. That way, users of *Unsafe
still have some reasonable means to guard themselves against errors.

(FLAG_debug_code is hugely helpful for testing, since it allows us to run our
entire test suite (+ all the fuzzers) and check some of the implicit
assumptions.)

So: LoadInternalFieldChecked does all the checks, and returns Undefined, as it
currently does. LoadInternalFieldUnchecked does - if FLAG_debug_code is true -
the same checks, but CRASH()es when they fail. (This is the signal that fuzzers
+ test infrastructure need.) And if !FLAG_debug_code, it would not do any
checks.

You'll probably want to factor out the actual generating code into suitable
methods.

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler.h
File src/fast-accessor-assembler.h (right):

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler...
src/fast-accessor-assembler.h:58: // Unsafe but fast version, does NOT perform
any sanity checks.
nitpick: Your description is correct, but unnecessarily dramatic. :) How about
this:

Loads internal field and assumes the object is indeed a valid API object with
the proper internal fields present.

The intended use is to call this on an object whose structure has already been
checked previously, e.g. the accessor's receiver, which is map-checked before
the fast accessor is called on it. Using this on an arbitrary object will result
in unsafe memory accesses.

https://codereview.chromium.org/2186593002/diff/1/test/cctest/test-api-fast-a...
File test/cctest/test-api-fast-accessor-builder.cc (right):

https://codereview.chromium.org/2186593002/diff/1/test/cctest/test-api-fast-a...
test/cctest/test-api-fast-accessor-builder.cc:169:
TEST(FastAccessorLoadInternalFieldUnsafe) {
nitpick: If we add the Unsafe-with-crash-when-FLAG_debug_code-is-true option,
we'll have a third variant of this test. If so, it'd probably be best to factor
this out into helper function(s). (I'll leave the decision to you, though.)

[Alfonso, 2016-07-28 14:39:13]

PTAL

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler.cc
File src/fast-accessor-assembler.cc (right):

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler...
src/fast-accessor-assembler.cc:94: FastAccessorAssembler::ValueId
FastAccessorAssembler::LoadInternalFieldUnsafe(
On 2016/07/27 09:50:56, vogelheim wrote:
> I briefly discussed handling the LoadInternalField[Unsafe] variants. How about
> we change it like this:
> 
> 1, we'll have both versions, LoadInternalFieldChecked and
> LoadInternalFieldUnsafe.
> 
> 2, There's a FLAG_debug_code, that we use elsewhere to instruct the compiler
to
> emit additional runtime checks (and crash if they fail). If this is set, we'll
> still check the assumptions in the *Unsafe case. That way, users of *Unsafe
> still have some reasonable means to guard themselves against errors.
> 
> (FLAG_debug_code is hugely helpful for testing, since it allows us to run our
> entire test suite (+ all the fuzzers) and check some of the implicit
> assumptions.)
> 
> So: LoadInternalFieldChecked does all the checks, and returns Undefined, as it
> currently does. LoadInternalFieldUnchecked does - if FLAG_debug_code is true -
> the same checks, but CRASH()es when they fail. (This is the signal that
fuzzers
> + test infrastructure need.) And if !FLAG_debug_code, it would not do any
> checks.
> 
> You'll probably want to factor out the actual generating code into suitable
> methods.
> 

The C++ counterpart returns NULL (instead of undefined) if the argument is not a
JSObject.
Refactoring into smaller composable parts.

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler.h
File src/fast-accessor-assembler.h (right):

https://codereview.chromium.org/2186593002/diff/1/src/fast-accessor-assembler...
src/fast-accessor-assembler.h:58: // Unsafe but fast version, does NOT perform
any sanity checks.
On 2016/07/27 09:50:56, vogelheim wrote:
> nitpick: Your description is correct, but unnecessarily dramatic. :) How about
> this:
> 
> Loads internal field and assumes the object is indeed a valid API object with
> the proper internal fields present.
> 
> The intended use is to call this on an object whose structure has already been
> checked previously, e.g. the accessor's receiver, which is map-checked before
> the fast accessor is called on it. Using this on an arbitrary object will
result
> in unsafe memory accesses.

Acknowledged. I lost objectivity.

https://codereview.chromium.org/2186593002/diff/1/test/cctest/test-api-fast-a...
File test/cctest/test-api-fast-accessor-builder.cc (right):

https://codereview.chromium.org/2186593002/diff/1/test/cctest/test-api-fast-a...
test/cctest/test-api-fast-accessor-builder.cc:169:
TEST(FastAccessorLoadInternalFieldUnsafe) {
On 2016/07/27 09:50:56, vogelheim wrote:
> nitpick: If we add the Unsafe-with-crash-when-FLAG_debug_code-is-true option,
> we'll have a third variant of this test. If so, it'd probably be best to
factor
> this out into helper function(s). (I'll leave the decision to you, though.)

Refactoring away the common part for the 3 tests.

[Alfonso, 2016-07-28 14:39:56]

Patchset #2 (id:20001) has been deleted

[vogelheim, 2016-08-08 11:47:22]

lgtm, I like this.

Some tiny nitpicks though, about comments & formatting. :)

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
File src/fast-accessor-assembler.cc (right):

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.cc:82: // Deliberately crash.
Hmm. Actually, we force a debug break (which terminates the program if there's
no debugger attached). So I thought change the comment to, "// Force debug
break." But then, that's exactly what the code actually says, so maybe just drop
the comment.

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.cc:167: CodeStubAssembler::Label
deferred(assembler_.get(),
This is a good change, but unrelated to what the CL description says. In this
case, I'd either make it a separate CL or, if that's burdensome, just add a
1-line comment to the CL description. (E.g.: "Also use deferred label for
function callbacks, to prevent a stack trace being built in cases where it's not
necessary.")

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
File src/fast-accessor-assembler.h (right):

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.h:88: void CheckIsJSObjectOrJump(ValueId value,
LabelId label_id);
nitpick: Maybe add empty line. The previous 4 methods are a group of closely
related functions (translating API-IDs to Node*), while this one is quite
different.

[Alfonso, 2016-08-08 13:03:29]

Fixed nits, PTAL.

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
File src/fast-accessor-assembler.cc (right):

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.cc:82: // Deliberately crash.
On 2016/08/08 11:47:22, vogelheim wrote:
> Hmm. Actually, we force a debug break (which terminates the program if there's
> no debugger attached). So I thought change the comment to, "// Force debug
> break." But then, that's exactly what the code actually says, so maybe just
drop
> the comment.

Acknowledged.

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.cc:167: CodeStubAssembler::Label
deferred(assembler_.get(),
On 2016/08/08 11:47:22, vogelheim wrote:
> This is a good change, but unrelated to what the CL description says. In this
> case, I'd either make it a separate CL or, if that's burdensome, just add a
> 1-line comment to the CL description. (E.g.: "Also use deferred label for
> function callbacks, to prevent a stack trace being built in cases where it's
not
> necessary.")

Reverting this, even if is the logical thing to do, there's no real gain (in the
benchmarks). I tried 'deferring' some unlikely paths and performance actually
regressed. The benchmarks are slightly biased towards a specific behavior, so
I'd like collect more data in order to have a solid argument to support this
optimization (or not).

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
File src/fast-accessor-assembler.h (right):

https://codereview.chromium.org/2186593002/diff/60001/src/fast-accessor-assem...
src/fast-accessor-assembler.h:88: void CheckIsJSObjectOrJump(ValueId value,
LabelId label_id);
On 2016/08/08 11:47:22, vogelheim wrote:
> nitpick: Maybe add empty line. The previous 4 methods are a group of closely
> related functions (translating API-IDs to Node*), while this one is quite
> different.

Acknowledged.

[Alfonso, 2016-08-10 11:05:34]

The CQ bit was checked by peterssen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 11:05:44]

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-10 11:37:57]

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

[commit-bot: I haz the power, 2016-08-10 11:37:58]

Dry run: This issue passed the CQ dry run.

[Alfonso, 2016-08-10 11:51:25]

The CQ bit was checked by peterssen@google.com

[Alfonso, 2016-08-10 11:51:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2186593002/#ps100001
(title: "Revert 'deferred' label")

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

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 11:53:58]

Description was changed from

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
==========

to

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
==========

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

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-10 11:54:17]

Description was changed from

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898
==========

to

==========
Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898

Committed: https://crrev.com/d073777bbf60df574ad0ba4b6069c90752d01ff1
Cr-Commit-Position: refs/heads/master@{#38535}
==========

[commit-bot: I haz the power, 2016-08-10 11:54:18]

Patchset 5 (id:??) landed as
https://crrev.com/d073777bbf60df574ad0ba4b6069c90752d01ff1
Cr-Commit-Position: refs/heads/master@{#38535}