Closed Bug 1460381 Opened 2 years ago Closed 2 years ago

Support sealed and non-extensible dense elements

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [qf])

Attachments

(4 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8974486 - Flags: review?(andrebargull)
Attached patch PatchSplinter Review
Bah, forgot to qref some of my test files.
Attachment #8974486 - Attachment is obsolete: true
Attachment #8974486 - Flags: review?(andrebargull)
Attachment #8974489 - Flags: review?(andrebargull)
Oh NativeObject::sparsifyDenseElements can be removed now \o/
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.
Attachment #8974489 - Flags: review?(andrebargull) → review+
Duplicate of this bug: 1297577
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.
(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.
Addresses the review comments and has some minor cleanup + fixes a few small bugs.
Attachment #8974932 - Flags: review?(andrebargull)
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
Attachment #8974932 - Flags: review?(andrebargull) → review+
Attached file 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.)
Whiteboard: [qf]
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46f3ba0c766
Support sealed and non-extensible dense elements on native objects. r=anba
Attached patch Follow-upSplinter Review
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.
Attachment #8975243 - Flags: review?(andrebargull)
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.
Attachment #8975243 - Flags: review?(andrebargull) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aedaedf623ee
Follow-up to fix test bustage and add some more tests. r=anba
I landed this, because I am pretty sure this was actually the cause for the cgc test failure in bug 1459940 comment 6.
https://hg.mozilla.org/mozilla-central/rev/b46f3ba0c766
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1461167
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..
Depends on: 1461272
Depends on: 1466363
You need to log in before you can comment on or make changes to this bug.