Use Cells to check prototype chain validity (disabled by default).

Use Cells to check prototype chain validity (disabled by default).

The cells are stored on prototypes (in their map's PrototypeInfo). When a prototype object changes its map, then both its own validity cell and those of all "downstream" prototypes are invalidated; handlers for a given receiver embed the currently valid cell for that receiver's prototype during their compilation and check it on execution.

Committed: https://crrev.com/0179ec57975c5063bbecd98e11cdda77ddad8996
Cr-Commit-Position: refs/heads/master@{#27845}

[Jakob Kummerow, 2015-02-10 14:49:39]

jkummerow@chromium.org changed reviewers:
+ verwaest@chromium.org

[Jakob Kummerow, 2015-02-10 14:49:39]

PTAL.

I'll get started on the platform ports.

[Toon Verwaest, 2015-02-16 17:16:40]

Looking good.

As discussed, rather than caching the values on the object somewhere under a
symbol, lets use the "transition array" field in the map to store a struct
containing both the validity cell and the dependency weakmap. That should clean
up the design a little. We can use the "is_prototype_map()" bit to distinguish
the meaning of the "transition array" entry.

[Toon Verwaest, 2015-04-07 09:28:38]

lgtm. You can ignore the handler-compiler comment for now, I think we should
just do it in the future. But maybe better to do it once this is the default
mode.

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
File src/ic/arm/handler-compiler-arm.cc (right):

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:427: DCHECK(validity_cell->value() ==
Smi::FromInt(Map::kPrototypeChainValid));
DCHECK_EQ

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:439: __ ldr(scratch1,
FieldMemOperand(object_reg, HeapObject::kMapOffset));
I think you just want to rewrite this code (and its client). Rather than
dynamically loading the right prototype, you want to verify the validity cell
for that prototype. Additionally here you need to check that the current context
matches the expected one (via esi/cp/rsi/...).

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:492: __ LoadWeakValue(reg,
isolate()->factory()->NewWeakCell(current), miss);
In the (near) future we should just drop support for dictionary-mode prototypes
in the IC.

https://codereview.chromium.org/908213002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/908213002/diff/60001/src/objects.cc#newcode10056
src/objects.cc:10056: prototype =
handle(JSObject::cast(prototype->map()->prototype()), isolate);
PrototypeIterator

[Jakob Kummerow, 2015-04-14 09:41:57]

Toon: thanks for the review, addressed your comments.

MIPS: This is ready for landing, please be so kind to prepare the ports.

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
File src/ic/arm/handler-compiler-arm.cc (right):

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:427: DCHECK(validity_cell->value() ==
Smi::FromInt(Map::kPrototypeChainValid));
On 2015/04/07 09:28:37, Toon Verwaest wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:439: __ ldr(scratch1,
FieldMemOperand(object_reg, HeapObject::kMapOffset));
On 2015/04/07 09:28:38, Toon Verwaest wrote:
> I think you just want to rewrite this code (and its client). Rather than
> dynamically loading the right prototype, you want to verify the validity cell
> for that prototype. Additionally here you need to check that the current
context
> matches the expected one (via esi/cp/rsi/...).

Acknowledged.

https://codereview.chromium.org/908213002/diff/60001/src/ic/arm/handler-compi...
src/ic/arm/handler-compiler-arm.cc:492: __ LoadWeakValue(reg,
isolate()->factory()->NewWeakCell(current), miss);
On 2015/04/07 09:28:37, Toon Verwaest wrote:
> In the (near) future we should just drop support for dictionary-mode
prototypes
> in the IC.

Acknowledged.

https://codereview.chromium.org/908213002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/908213002/diff/60001/src/objects.cc#newcode10056
src/objects.cc:10056: prototype =
handle(JSObject::cast(prototype->map()->prototype()), isolate);
On 2015/04/07 09:28:38, Toon Verwaest wrote:
> PrototypeIterator

Done.

[paul.l..., 2015-04-14 16:35:24]

On 2015/04/14 09:41:57, Jakob wrote:
> MIPS: This is ready for landing, please be so kind to prepare the ports.

Jakob, with this CL on ToT now, with the flag on, there are lots of crashes in
release mode, and asserts hit in debug (ia32 & x64):
# Fatal error in .././src/objects-inl.h, line 3427
# Check failed: !v8::internal::FLAG_enable_slow_asserts ||
(object->IsPrototypeInfo()).

one example: test/mjsunit/with-function-expression.js

So we can test without the flag, but most of the new code is behind the flag.

[balazs.kilvady, 2015-04-14 16:58:50]

On 2015/04/14 16:35:24, paul.l... wrote:
> On 2015/04/14 09:41:57, Jakob wrote:
> > MIPS: This is ready for landing, please be so kind to prepare the ports.
> 
> Jakob, with this CL on ToT now, with the flag on, there are lots of crashes in
> release mode, and asserts hit in debug (ia32 & x64):
> # Fatal error in .././src/objects-inl.h, line 3427
> # Check failed: !v8::internal::FLAG_enable_slow_asserts ||
> (object->IsPrototypeInfo()).
> 
> one example: test/mjsunit/with-function-expression.js
> 
> So we can test without the flag, but most of the new code is behind the flag.

MIPS port is ready: https://codereview.chromium.org/1080263004/

[Jakob Kummerow, 2015-04-14 17:16:00]

On 2015/04/14 16:35:24, paul.l... wrote:
> On 2015/04/14 09:41:57, Jakob wrote:
> > MIPS: This is ready for landing, please be so kind to prepare the ports.
> 
> Jakob, with this CL on ToT now, with the flag on, there are lots of crashes in
> release mode, and asserts hit in debug (ia32 & x64):
> # Fatal error in .././src/objects-inl.h, line 3427
> # Check failed: !v8::internal::FLAG_enable_slow_asserts ||
> (object->IsPrototypeInfo()).
> 
> one example: test/mjsunit/with-function-expression.js
> 
> So we can test without the flag, but most of the new code is behind the flag.

Thanks for the heads-up; that's not intended. I ran tests with the flag on
earlier today and didn't see any failures. I'll investigate tomorrow.

While writing this, I had an idea: did you pass the flag on the command-line?
Unfortunately that's not supported, as code in the snapshot has to agree with
code generated later. So to toggle the flag you'll have to edit
flag-definitions.h and recompile V8. Does that fix the failures you were seeing?

The plan is to land the CL with the flag off first, and then turn it on soon
after (as soon as the buildbots are green, probably), so that reverting is easy.
So for now, tests should pass both with and without the flag.

[paul.l..., 2015-04-14 17:18:42]

On 2015/04/14 17:16:00, Jakob wrote:
> While writing this, I had an idea: did you pass the flag on the command-line?
> Unfortunately that's not supported, as code in the snapshot has to agree with
> code generated later. So to toggle the flag you'll have to edit
> flag-definitions.h and recompile V8. Does that fix the failures you were
seeing?

Yes, I passed flag on command line -- thanks for mentioning this. Will retest
now, suspect it will be fine.

[paul.l..., 2015-04-15 01:58:31]

On 2015/04/14 17:18:42, paul.l... wrote:
> On 2015/04/14 17:16:00, Jakob wrote:
> > edit flag-definitions.h and recompile V8. Does that fix the failures you
were
> > seeing?

Yes, it's fine on all arch's I tested, including MIPS when you merge in
https://codereview.chromium.org/1080263004/

[Jakob Kummerow, 2015-04-15 09:41:56]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-04-15 09:41:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/908213002/#ps140001
(title: "add MIPS ports")

[commit-bot: I haz the power, 2015-04-15 09:42:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908213002/140001

[commit-bot: I haz the power, 2015-04-15 10:37:07]

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

[commit-bot: I haz the power, 2015-04-15 10:37:08]

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

[Jakob Kummerow, 2015-04-15 11:39:15]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-04-15 11:39:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/908213002/#ps160001
(title: "fix arm64 typo (and rebase, sorry)")

[commit-bot: I haz the power, 2015-04-15 11:39:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908213002/160001

[commit-bot: I haz the power, 2015-04-15 12:53:34]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-15 12:53:50]

Patchset 9 (id:??) landed as
https://crrev.com/0179ec57975c5063bbecd98e11cdda77ddad8996
Cr-Commit-Position: refs/heads/master@{#27845}