1460381 - Support sealed and non-extensible dense elements

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Native objects can have frozen elements but we still sparsify when Object.preventExtensions or Object.seal is used. Because frozen implies sealed and non-extensible, it isn't too hard to support sealed and non-extensible dense elements as well.

This fixes some potential perf cliffs, but my main reason for doing this is that it simplifies the code in a number of places. The way PreventExtensions and SetIntegrityLevel currently interact has caused a number of OOM issues.

Most of the places where we used to check denseElementsAreFrozen now check isExtensible or denseElementsAreSealed.

Unfortunately our test coverage of non-extensible/sealed/frozen objects isn't great. I added a number of tests; if you can think of more edge cases I'm happy to add more.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Bah, forgot to qref some of my test files.

Comment 2

[Jan de Mooij [:jandem]]

Oh NativeObject::sparsifyDenseElements can be removed now \o/

Comment 3

[André Bargull [:anba]]

Comment on attachment 8974489 [details] [diff] [review]
Patch

Review of attachment 8974489 [details] [diff] [review]:
-----------------------------------------------------------------

A really cool change, great to see the performance cliff for making objects with elements non-extensible/sealed go away. And I feel much more comfortable with this approach compared to the one in the other patch!


(In reply to Jan de Mooij [:jandem] from comment #0)
> Unfortunately our test coverage of non-extensible/sealed/frozen objects
> isn't great. I added a number of tests; if you can think of more edge cases
> I'm happy to add more.

So it's not yet time to close bug 598390. :-)

https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/vm/NativeObject.h#174-177
- Update to mention "sealed" elements?

https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/vm/ArrayObject-inl.h#24
- Can we call |denseElementsAreSealed()| here instead of |denseElementsAreFrozen()|?
- Maybe also add the denseElementsAreFrozen/Sealed check to ArrayObject::setLengthInt32()?

https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/vm/JSObject.cpp#525
- Could now be changed to |MOZ_ASSERT(!nobj->denseElementsAreCopyOnWrite())|.

https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/js/src/builtin/Array.cpp#2382
- Also test for sealed elements to match the condition in CodeGenerator::emitArrayPopShift()?

::: js/src/jit/CodeGenerator.cpp
@@ +9632,2 @@
>      Address elementFlags(elementsTemp, ObjectElements::offsetOfFlags());
> +    Imm32 bits(ObjectElements::NONWRITABLE_ARRAY_LENGTH | ObjectElements::SEALED);

Checking SEALED isn't strictly required here, because SEALED currently implies NONWRITABLE_ARRAY_LENGTH, doesn't it?. But OTOH it probably doesn't hurt to check it, too.

::: js/src/jit/IonBuilder.cpp
@@ +9193,5 @@
>  
>      // Use MStoreElementHole if this SETELEM has written to out-of-bounds
>      // indexes in the past. Otherwise, use MStoreElement so that we can hoist
>      // the initialized length and bounds check.
>      // If an object may have been frozen, no previous expectation hold and we

s/frozen/made non-extensible/

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +1934,5 @@
> +    // Check for frozen elements. We have to check this here because we attach
> +    // this stub also for non-extensible objects, and these can become frozen
> +    // without triggering a Shape change.
> +    Address flags(scratch1, ObjectElements::offsetOfFlags());
> +    masm.branchTest32(Assembler::NonZero, flags, Imm32(ObjectElements::FROZEN), failure->label());

Hmm, so assuming the comment isn't only a false flag operation to confuse readers ;-) , it made me wonder why we check for FROZEN in BaselineCacheIRCompiler::emitStoreDenseElementHole() [1] and BaselineCacheIRCompiler::emitArrayPush() [2] ?
CacheIR only seems to attach these stubs for extensible objects [3,4], so is it actually possible to have a frozen object when the code for the baseline CacheIR case is executed? 


[1] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/BaselineCacheIRCompiler.cpp#1475
[2] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/BaselineCacheIRCompiler.cpp#1622
[3] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/CacheIR.cpp#3535-3536
[4] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/CacheIR.cpp#4392-4394

::: js/src/jit/MIR.h
@@ +10137,5 @@
>    : public MQuaternaryInstruction,
>      public MStoreElementCommon,
>      public MixPolicy<SingleObjectPolicy, NoFloatPolicy<3> >::Data
>  {
>      bool strict_;

This field seems to be unused now.

::: js/src/vm/JSObject.cpp
@@ +643,5 @@
> +            if (!nobj->denseElementsAreSealed()) {
> +                *result = false;
> +                return true;
> +            }
> +            // Unless the frozen flag is set, dense elements are writable.

Nit: blank line before the comment.

::: js/src/vm/NativeObject.cpp
@@ +927,2 @@
>      MOZ_ASSERT(canHaveNonEmptyElements());
> +    MOZ_ASSERT(isExtensible());

|isExtensible()| is checked twice here.

::: js/src/vm/NativeObject.h
@@ +361,5 @@
>      bool isFrozen() const {
>          return flags & FROZEN;
>      }
> +
> +    void seal() {

Can we make this private to ensure it can only be called through FreezeOrSeal()?

@@ +371,1 @@
>      void freeze() {

Also private.

Comment 5

[Jan de Mooij [:jandem]]

Great review comments, thanks!

(In reply to André Bargull [:anba] from comment #3)
> Checking SEALED isn't strictly required here, because SEALED currently
> implies NONWRITABLE_ARRAY_LENGTH, doesn't it?.

It doesn't, a sealed array has a writable length. (The length property is non-configurable so "freeze" is the only operation here that affects it, IIUC.)

> BaselineCacheIRCompiler::emitStoreDenseElementHole() [1] and
> BaselineCacheIRCompiler::emitArrayPush() [2] ?
> CacheIR only seems to attach these stubs for extensible objects [3,4], so is
> it actually possible to have a frozen object when the code for the baseline
> CacheIR case is executed? 

Correct, it's unnecessary. I'll post a follow-up patch to clean that up. There's also a bug in the patch that I realized yesterday, will fix it at the same time.

Comment 6

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to André Bargull [:anba] from comment #3)
> > Checking SEALED isn't strictly required here, because SEALED currently
> > implies NONWRITABLE_ARRAY_LENGTH, doesn't it?.
> 
> It doesn't, a sealed array has a writable length. (The length property is
> non-configurable so "freeze" is the only operation here that affects it,
> IIUC.)

Oops, yes.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Follow-up changes

Addresses the review comments and has some minor cleanup + fixes a few small bugs.

Comment 8

[André Bargull [:anba]]

Comment on attachment 8974932 [details] [diff] [review]
Follow-up changes

Review of attachment 8974932 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! 


For a follow-up bug:
We should probably change |CanAttachAddElement(JSObject*, bool)| [1] to take |NativeObject*| instead of |JSObject*|, because both callers are always NativeObjects [2,3].

[1] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/CacheIR.cpp#3480
[2] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/CacheIR.cpp#3560
[3] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/CacheIR.cpp#4385

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Micro-benchmarks

Weird, I had assumed JSC and V8 always optimized this, but they have pretty bad cliffs too. For the attached preventExtensions/seal/freeze tests:

Chrome Canary:   704 /  721 /  88
Safari:          591 /  585 / 184
Nightly before: 1027 / 1024 /  13
Nightly after:    19 /   19 /  10

(The freeze test is faster because it doesn't do a SetElem.)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46f3ba0c766
Support sealed and non-extensible dense elements on native objects. r=anba

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Follow-up

I was looking more into sparsifyDenseElement and ended up writing tests for defineProperty. We could run into some bogus asserts there but otherwise it works as expected.

This also fixes CanAttachAddElement to take NativeObject* instead of JSObject*, as suggested.

Comment 12

[André Bargull [:anba]]

Comment on attachment 8975243 [details] [diff] [review]
Follow-up

Review of attachment 8975243 [details] [diff] [review]:
-----------------------------------------------------------------

Quick r+ because I've read that the assertion effects other people to land their code.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aedaedf623ee
Follow-up to fix test bustage and add some more tests. r=anba

Comment 14

[Tom S [:evilpie]]

I landed this, because I am pretty sure this was actually the cause for the cgc test failure in bug 1459940 comment 6.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b46f3ba0c766

Comment 16

[Jan de Mooij [:jandem]]

Thanks, Tom. That push did not add the test file, I'll push it separately.

In hindsight I should probably have backed this out after I noticed there were some invalid asserts..

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a1f40307d8
Add more tests. r=anba

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/aedaedf623ee
https://hg.mozilla.org/mozilla-central/rev/09a1f40307d8