[runtime] PreventExtensionsWithTransition: before adding the new

[runtime] PreventExtensionsWithTransition: before adding the new
transition, check to see if we have already done this transition.

BUG=v8:6450
Review-Url: https://codereview.chromium.org/2915863004
Cr-Commit-Position: refs/heads/master@{#46129}
Committed: https://chromium.googlesource.com/v8/v8/+/6681949808bb89675ab64c95a237ae76955e191b

[Benedikt Meurer, 2017-06-01 09:44:11]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org, ishell@chromium.org, verwaest@chromium.org

[Benedikt Meurer, 2017-06-01 09:44:12]

Igor, Toon, please take a look.

[Toon Verwaest, 2017-06-01 18:50:52]

Thanks for the CL Kris! The change makes sense to me, but I'm wondering whether
we shouldn't just check whether we're already in the correct state. This is
obviously faster but won't catch all cases...

[kris.selden, 2017-06-01 21:27:07]

On 2017/06/01 18:50:52, Toon Verwaest wrote:
> Thanks for the CL Kris! The change makes sense to me, but I'm wondering
whether
> we shouldn't just check whether we're already in the correct state. This is
> obviously faster but won't catch all cases...

For sealed, I could check the kExtensible bit before searching but there is no
frozen bit, and if there are any properties, is there a quick check for frozen? 
One thought I had was CopyForPreventExtensions could be a copy on write, only
return a new_map if the descriptors needed to change.

Another simple fix would just be to call TestIntegrityLevel, but it is only
re-adding transitions that is leaking memory.

[kris.selden, 2017-06-01 21:35:00]

On 2017/06/01 21:27:07, kris.selden wrote:
> On 2017/06/01 18:50:52, Toon Verwaest wrote:
> > Thanks for the CL Kris! The change makes sense to me, but I'm wondering
> whether
> > we shouldn't just check whether we're already in the correct state. This is
> > obviously faster but won't catch all cases...
> 
> For sealed, I could check the kExtensible bit before searching but there is no
> frozen bit, and if there are any properties, is there a quick check for
frozen? 
> One thought I had was CopyForPreventExtensions could be a copy on write, only
> return a new_map if the descriptors needed to change.
> 
> Another simple fix would just be to call TestIntegrityLevel, but it is only
> re-adding transitions that is leaking memory.

could I add a bit for frozen for when a map transitions via
PreventExtensionsWithTransition?

[Toon Verwaest, 2017-06-06 15:25:57]

There is indeed no quick way to check it, but the complexity if making it frozen
equals the complexity of checking it. You just save the memory overhead by not
retransitioning. I think it's worth just doing the expensive check; until we
have a good reason to optimize beyond that...

Note that the freeze-transition isn't guaranteed to be at the end. While the
user can't add new properties to the object, V8 still can and would in certain
cases. Private symbols can always be added, and will be if e.g., the object is
used as a key in a Map (to store the identity hash).

[stefan.penner, 2017-06-06 16:21:12]

> You just save the memory overhead by not retransitioning

It is quite surprising that `Object.freeze` is not idempotent. This confusion
lead us to introduce a development aide, which very unexpectedly resulted in a
poor development experience. More context here ->
https://github.com/emberjs/ember.js/issues/15290

> I think it's worth just doing the expensive check; until we have a good reason
to optimize beyond that...

Would the above provided link be a good reason?

[Toon Verwaest, 2017-06-07 09:19:23]

I think I didn't formulate my review very clearly, since I think you
misunderstood my comment as something along the lines of "don't do this, it just
saves memory".

What I meant was: do save memory by not retransitioning, but don't locally
optimize the performance of figuring out whether we are retransitioning by
walking 1 step back. The sentence "you just save the memory overhead" was meant
to affirm that you should do this; not that it isn't really relevant :)

So back to my suggestion: I think you should replace the hack that checks the
incoming transition as a quick-check with a full-blown TestIntegrityLevel, which
avoids doing the transition if we're already at the right level. For performance
reasons we probably want to split out a JSObject version that's less general
than the current JSReceiver::TestIntegrityLevel; and optimized for both
dictionary and fast-mode objects. As a final optimization it can also peak the
transition.

By separating these two concerns you avoid the memory overhead in general, which
you currently don't (the transition might not be at the end and might be
interleaved with other similar transitions). And the regular TestIntegrityLevel
(e.g., Object.isFrozen) will also benefit from your performance optimizations.

[kris.selden, 2017-06-12 19:50:10]

On 2017/06/07 09:19:23, Toon Verwaest wrote:
> I think I didn't formulate my review very clearly, since I think you
> misunderstood my comment as something along the lines of "don't do this, it
just
> saves memory".
> 
> What I meant was: do save memory by not retransitioning, but don't locally
> optimize the performance of figuring out whether we are retransitioning by
> walking 1 step back. The sentence "you just save the memory overhead" was
meant
> to affirm that you should do this; not that it isn't really relevant :)
> 
> So back to my suggestion: I think you should replace the hack that checks the
> incoming transition as a quick-check with a full-blown TestIntegrityLevel,
which
> avoids doing the transition if we're already at the right level. For
performance
> reasons we probably want to split out a JSObject version that's less general
> than the current JSReceiver::TestIntegrityLevel; and optimized for both
> dictionary and fast-mode objects. As a final optimization it can also peak the
> transition.
> 
> By separating these two concerns you avoid the memory overhead in general,
which
> you currently don't (the transition might not be at the end and might be
> interleaved with other similar transitions). And the regular
TestIntegrityLevel
> (e.g., Object.isFrozen) will also benefit from your performance optimizations.

I think I've addressed the concerns above, is there anything more I can do to
move this patch along?  There are a few common places integrity is checked like
array shift that would benefit from a fast path in the test.

[Toon Verwaest, 2017-06-13 09:57:06]

Looks good. Some final minor comments.

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7426
src/objects.cc:7426: // prevent memory leaks by adding unnecessary transitions
Prevent ... *not* adding ... transitions. :)

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7530
src/objects.cc:7530: ElementsAccessor* accessor = object->GetElementsAccessor();
If we only support fast elements, finding any element makes it false.

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7553
src/objects.cc:7553: Handle<JSReceiver> receiver =
Handle<JSReceiver>::cast(object);
We definitely want to share this slow path with the code for JSReceiver above.

[Toon Verwaest, 2017-06-16 10:03:02]

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7530
src/objects.cc:7530: ElementsAccessor* accessor = object->GetElementsAccessor();
On 2017/06/13 09:57:06, Toon Verwaest wrote:
> If we only support fast elements, finding any element makes it false.

Come to think of it; if we ever followed the fast transition, we always end up
with slow elements. See CopyForPreventExtensions; it always sets the elements
kind to something slow (except typed arrays).

[kris.selden, 2017-06-16 13:48:13]

On 2017/06/16 10:03:02, Toon Verwaest wrote:
> https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7530
> src/objects.cc:7530: ElementsAccessor* accessor =
object->GetElementsAccessor();
> On 2017/06/13 09:57:06, Toon Verwaest wrote:
> > If we only support fast elements, finding any element makes it false.
> 
> Come to think of it; if we ever followed the fast transition, we always end up
> with slow elements. See CopyForPreventExtensions; it always sets the elements
> kind to something slow (except typed arrays).

Yes, dictionary elements, I noticed this last night, I have a cleaned up patch
coming shortly

[kris.selden, 2017-06-16 22:32:12]

I've addressed the comments, this is ready for review.

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7426
src/objects.cc:7426: // prevent memory leaks by adding unnecessary transitions
On 2017/06/13 09:57:05, Toon Verwaest wrote:
> Prevent ... *not* adding ... transitions. :)

Done.

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7530
src/objects.cc:7530: ElementsAccessor* accessor = object->GetElementsAccessor();
On 2017/06/16 10:03:02, Toon Verwaest wrote:
> On 2017/06/13 09:57:06, Toon Verwaest wrote:
> > If we only support fast elements, finding any element makes it false.
> 
> Come to think of it; if we ever followed the fast transition, we always end up
> with slow elements. See CopyForPreventExtensions; it always sets the elements
> kind to something slow (except typed arrays).

I think what I did addresses this but I need your review.

https://codereview.chromium.org/2915863004/diff/20001/src/objects.cc#newcode7553
src/objects.cc:7553: Handle<JSReceiver> receiver =
Handle<JSReceiver>::cast(object);
On 2017/06/13 09:57:05, Toon Verwaest wrote:
> We definitely want to share this slow path with the code for JSReceiver above.

Done.

[Toon Verwaest, 2017-06-20 08:43:23]

I like this version a lot! Added some (final, I suppose) comments.

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7518
src/objects.cc:7518: FOR_WITH_HANDLE_SCOPE(isolate, uint32_t, j = 0, j, j <
capacity, j++, {
Since you don't create handles here you don't need a FOR_WITH_HANDLE_SCOPE. Just
use a regular for instead.

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7520
src/objects.cc:7520: if (!dict->IsKey(isolate, k)) continue;
You also need to check whether the key is "deleted". In the case of global
properties we actually create "deleted" entries so we can cache property lookups
that find properties behind the global object.

Below I describe that we probably shouldn't support the global object on this
fast path, but even so I think we should add the IsDeleted check here to avoid
forgetting to add it later if we ever use this for other purposes.

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7578
src/objects.cc:7578: if (object->IsJSObject()) {
object->instance_type() > LAST_CUSTOM_ELEMENTS_RECEIVER

This makes sure that objects with indexed and named interceptors are dealt with
generically (e.g., localStorage); and we only take the fast path in case we only
need to deal with regular-type properties are you're doing below.

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7596
src/objects.cc:7596: if (object->IsAccessCheckNeeded() &&
If you only support > LAST_CUSTOM_ELEMENTS_RECEIVER as described above, this
shouldn't be necessary.

I presume the performance of Object.freeze(window) isn't of vital importance ...
:)

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7601
src/objects.cc:7601: if (object->IsJSGlobalProxy()) {
And in that case you can also drop this.

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7616
src/objects.cc:7616: if (!TestElementsIntegrityLevel(*object, level)) {
return Just(!object->map()->is_extensible() && 
            TestElementsIntegrityLevel(*object, level) &&
            TestPropertiesIntegrityLevel(*object, level));

[kris.selden, 2017-06-20 19:16:21]

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/40001/src/objects.cc#newcode7518
src/objects.cc:7518: FOR_WITH_HANDLE_SCOPE(isolate, uint32_t, j = 0, j, j <
capacity, j++, {
On 2017/06/20 08:43:23, Toon Verwaest wrote:
> Since you don't create handles here you don't need a FOR_WITH_HANDLE_SCOPE.
Just
> use a regular for instead.

sorry, that was a bit of copy & paste from some other 'Fast' method.

[Toon Verwaest, 2017-06-20 19:25:37]

lgtm

[kris.selden, 2017-06-20 19:26:41]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 19:26:51]

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, 2017-06-20 19:46:29]

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

[commit-bot: I haz the power, 2017-06-20 19:46:30]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/27831)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)

[kris.selden, 2017-06-20 20:52:15]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 20:52:20]

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, 2017-06-20 20:55:06]

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

[commit-bot: I haz the power, 2017-06-20 20:55:07]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/27834)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[kris.selden, 2017-06-20 21:35:34]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 21:35:36]

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, 2017-06-20 21:53:47]

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

[commit-bot: I haz the power, 2017-06-20 21:53:48]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[kris.selden, 2017-06-20 23:06:04]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 23:06:08]

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, 2017-06-20 23:32:31]

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

[commit-bot: I haz the power, 2017-06-20 23:32:32]

Dry run: This issue passed the CQ dry run.

[kris.selden, 2017-06-21 17:29:11]

On 2017/06/20 19:25:37, Toon Verwaest wrote:
> lgtm

it's green now, last issue was filtering private symbols.

[Toon Verwaest, 2017-06-21 18:58:08]

https://codereview.chromium.org/2915863004/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2915863004/diff/100001/src/objects.cc#newcode...
src/objects.cc:7495: if (!dict->IsKey(isolate, dict->KeyAt(i)) ||
dict->IsDeleted(i)) continue;
Private symbols can also occur as keys in NameDictionaries. I presume this also
doesn't work in that case :s.

https://codereview.chromium.org/2915863004/diff/100001/src/objects.cc#newcode...
src/objects.cc:7507: DCHECK(map->instance_type() >
LAST_CUSTOM_ELEMENTS_RECEIVER);
DCHECK_LT(LAST_CUSTOM_ELEMENTS_RECEIVER, map->instance_type());

same below

[kris.selden, 2017-06-21 23:27:02]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 23:27:07]

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, 2017-06-22 01:32:49]

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

[commit-bot: I haz the power, 2017-06-22 01:32:50]

Dry run: Try jobs failed on following builders:
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[kris.selden, 2017-06-22 02:20:53]

The CQ bit was checked by kris.selden@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 02:21:00]

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, 2017-06-22 02:47:04]

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

[commit-bot: I haz the power, 2017-06-22 02:47:05]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-06-22 12:16:50]

lgtm, thanks!

[Toon Verwaest, 2017-06-22 12:16:55]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2017-06-22 12:17:03]

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, 2017-06-22 12:19:27]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498133815000830,
"parent_rev": "2b730f63356051b59c268fd932878800cf2cc3ad", "commit_rev":
"6681949808bb89675ab64c95a237ae76955e191b"}

[commit-bot: I haz the power, 2017-06-22 12:19:35]

Description was changed from

==========
[runtime] PreventExtensionsWithTransition: before adding the new
transition, check to see if we have already done this transition.

BUG=v8:6450
==========

to

==========
[runtime] PreventExtensionsWithTransition: before adding the new
transition, check to see if we have already done this transition.

BUG=v8:6450

Review-Url: https://codereview.chromium.org/2915863004
Cr-Commit-Position: refs/heads/master@{#46129}
Committed:
https://chromium.googlesource.com/v8/v8/+/6681949808bb89675ab64c95a237ae76955...
==========

[commit-bot: I haz the power, 2017-06-22 12:19:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/6681949808bb89675ab64c95a237ae76955...