Bug 171591 – putDirectIndex does not properly do defineOwnProperty

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 308882 [details]
WIP

I need to teach the JITs that put_by_val_direct on a typed array is not optimizable in the same way as put_by_val. We currently do the same optimization in baseline/DFG regardless of direct or not.

[Saam Barati]

<rdar://problem/31735695>

[Saam Barati]

Created attachment 308938 [details]
WIP

Might be done. Want to run on EWS.

[Build Bot]

Comment on attachment 308938 [details]
WIP

Attachment 308938 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3666318

New failing tests:
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-llint
stress/array-species-config-array-constructor.js.ftl-eager-no-cjit-b3o1
stress/array-species-config-array-constructor.js.ftl-no-cjit-validate-sampling-profiler
stress/array-species-config-array-constructor.js.no-cjit-collect-continuously
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-ftl-eager-no-cjit
stress/array-species-config-array-constructor.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-dfg-eager-no-cjit
stress/array-species-config-array-constructor.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-ftl-no-cjit
stress/array-species-config-array-constructor.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout
stress/array-species-config-array-constructor.js.default
stress/array-species-config-array-constructor.js.no-llint
stress/array-species-config-array-constructor.js.ftl-eager-no-cjit
stress/array-species-config-array-constructor.js.ftl-no-cjit-no-inline-validate
stress/array-species-config-array-constructor.js.ftl-no-cjit-b3o1
stress/array-species-config-array-constructor.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/class-syntax-string-and-numeric-names.js.layout-no-cjit
stress/array-species-config-array-constructor.js.dfg-eager
stress/array-species-config-array-constructor.js.dfg-maximal-flush-validate-no-cjit
stress/array-species-config-array-constructor.js.no-ftl
stress/array-species-config-array-constructor.js.dfg-eager-no-cjit-validate

[Build Bot]

Comment on attachment 308938 [details]
WIP

Attachment 308938 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3666401

New failing tests:
js/class-syntax-string-and-numeric-names.html

[Build Bot]

Created attachment 308945 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308938 [details]
WIP

Attachment 308938 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3666409

New failing tests:
js/class-syntax-string-and-numeric-names.html

[Build Bot]

Created attachment 308946 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308938 [details]
WIP

Attachment 308938 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3666415

New failing tests:
js/class-syntax-string-and-numeric-names.html

[Build Bot]

Created attachment 308948 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308938 [details]
WIP

Attachment 308938 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3666440

New failing tests:
js/class-syntax-string-and-numeric-names.html

[Build Bot]

Created attachment 308951 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Saam Barati]

*** Bug 159645 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 308984 [details]
patch

[Mark Lam]

Comment on attachment 308984 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        defineProperty({configurable:true, enumerable:true, writable:true}), however,

Run on sentence here.  I suggest putting a period before "however", and starting "However" as a new sentence.

> Source/JavaScriptCore/ChangeLog:26
> +        This patch fixes this issue by adding a whitelist of cell types that can
> +        go down putDirectIndex's fast path. Anything not in that whitelist will
> +        simply call into defineOwnProperty.

Did you benchmark this to make sure there's no perf impact due to checking the whitelist?

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:251
> +            return typedArrayResult(withSpeculation(Array::InBounds));

why not just fall thru to the case at line 253?  They do the same thing.

> Source/JavaScriptCore/jit/JITOperations.cpp:-590
> -        if (baseObject->canSetIndexQuicklyForPutDirect(index)) {
> -            baseObject->setIndexQuickly(callFrame->vm(), index, value);
> -            return;
> -        }

Why remove this?

> Source/JavaScriptCore/jit/JITOperations.cpp:589
>          // FIXME: This will make us think that in-bounds typed array accesses are actually
>          // out-of-bounds.

Is this still relevant?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1042
> -                result->putByIndexInline(exec, k, v, true);

if !canSetIndexQuickly(), putByIndexInline() will go thru the generic methodTable(exec->vm())->putByIndex(...) route i.e. can be overridden by Proxy, no?  However, putDirectIndex() below will not route thru the methodTable's putByIndex.  I see that you've changed putDirectIndexSlowOrBeyondVectorLength to route thru methodTable(vm)->defineOwnProperty() as needed.  But will this result in an observable difference in behavior?  If so, is it still spec compliant?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:399
> +            return typeError(exec, scope, shouldThrow, makeString("Attempting to store accessor property on a typed array at index: ", String::number(*index)));

Would this end up making the error string even if we don't need to throw?  If so, maybe you should change typeError() to a be a vararg template and just forward the construction cost to the throw case only.

[Saam Barati]

Comment on attachment 308984 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review

Thanks for taking a look. Comments below.

>> Source/JavaScriptCore/ChangeLog:14
>> +        defineProperty({configurable:true, enumerable:true, writable:true}), however,
> 
> Run on sentence here.  I suggest putting a period before "however", and starting "However" as a new sentence.

sounds good

>> Source/JavaScriptCore/ChangeLog:26
>> +        simply call into defineOwnProperty.
> 
> Did you benchmark this to make sure there's no perf impact due to checking the whitelist?

No. Will do before landing.

>> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:251
>> +            return typedArrayResult(withSpeculation(Array::InBounds));
> 
> why not just fall thru to the case at line 253?  They do the same thing.

sounds good

>> Source/JavaScriptCore/jit/JITOperations.cpp:-590
>> -        }
> 
> Why remove this?

It's wrong. See that I removed this function for this exact reason. It should be internal to putDirectIndex.

>> Source/JavaScriptCore/jit/JITOperations.cpp:589
>>          // out-of-bounds.
> 
> Is this still relevant?

Yeah it is. I think the repro situation is something like this: index is below vector length, but above public length.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:-1042
>> -                result->putByIndexInline(exec, k, v, true);
> 
> if !canSetIndexQuickly(), putByIndexInline() will go thru the generic methodTable(exec->vm())->putByIndex(...) route i.e. can be overridden by Proxy, no?  However, putDirectIndex() below will not route thru the methodTable's putByIndex.  I see that you've changed putDirectIndexSlowOrBeyondVectorLength to route thru methodTable(vm)->defineOwnProperty() as needed.  But will this result in an observable difference in behavior?  If so, is it still spec compliant?

putByIndex does Set(). putDirectIndex does defineOwnProperty. It's not spec compliant to have this to do Set(). It should've always been doing defineOwnProperty. One of my tests catches this exact case.
putDirectIndex will go through the method table's defineOwnProperty (that is what the heart of this patch does).

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:399
>> +            return typeError(exec, scope, shouldThrow, makeString("Attempting to store accessor property on a typed array at index: ", String::number(*index)));
> 
> Would this end up making the error string even if we don't need to throw?  If so, maybe you should change typeError() to a be a vararg template and just forward the construction cost to the throw case only.

Yeah I thought about doing something like this. It probably doesn't matter in practice, but something like your suggestion could work.

[Keith Miller]

Comment on attachment 308984 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review

>> Source/JavaScriptCore/jit/JITOperations.cpp:-590
>> -        }
> 
> Why remove this?

But I think there is a different bug here. Doesn't this mean that we will think anything code that does a putDirect with an uint32 accesses out of bounds? 

I think before we would have only thought it went out of bounds if it went past the vector length.

>> Source/JavaScriptCore/jit/JITOperations.cpp:589
>>          // out-of-bounds.
> 
> Is this still relevant?

Yeah, this still happens.

[Saam Barati]

(In reply to Keith Miller from comment #17)
> Comment on attachment 308984 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308984&action=review
> 
> >> Source/JavaScriptCore/jit/JITOperations.cpp:-590
> >> -        }
> > 
> > Why remove this?
> 
> But I think there is a different bug here. Doesn't this mean that we will
> think anything code that does a putDirect with an uint32 accesses out of
> bounds? 
> 
> I think before we would have only thought it went out of bounds if it went
> past the vector length.
Yeah I missed this. Perf tests should probably pick this up. I'll properly only set out of bounds when we would have used to. Maybe I'll rename the old function.
 
> 
> >> Source/JavaScriptCore/jit/JITOperations.cpp:589
> >>          // out-of-bounds.
> > 
> > Is this still relevant?
> 
> Yeah, this still happens.

[Saam Barati]

Comment on attachment 308984 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308984&action=review

>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:589
>>>>>          // out-of-bounds.
>>>> 
>>>> Is this still relevant?
>>> 
>>> Yeah it is. I think the repro situation is something like this: index is below vector length, but above public length.
>> 
>> Yeah, this still happens.
> 
> 

Sorry, you're correct, this comment is no longer relevant. I'm just going to switch the code to manually switch on indexing type.

[Saam Barati]

Created attachment 309017 [details]
patch

Going to run benchmarks now.

[Saam Barati]

Created attachment 309020 [details]
benchmark results

looks neutral

[Keith Miller]

Comment on attachment 309017 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309017&action=review

> Source/JavaScriptCore/jit/JITOperations.cpp:-594
> -        // FIXME: This will make us think that in-bounds typed array accesses are actually
> -        // out-of-bounds.
> -        // https://bugs.webkit.org/show_bug.cgi?id=149886

I don't understand how this is not relevant anymore. Don't all typed arrays have an Undecided indexing type? If so, wouldn't they always set out of bounds?

[Saam Barati]

(In reply to Keith Miller from comment #22)
> Comment on attachment 309017 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309017&action=review
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:-594
> > -        // FIXME: This will make us think that in-bounds typed array accesses are actually
> > -        // out-of-bounds.
> > -        // https://bugs.webkit.org/show_bug.cgi?id=149886
> 
> I don't understand how this is not relevant anymore. Don't all typed arrays
> have an Undecided indexing type? If so, wouldn't they always set out of
> bounds?

You're correct that we will mark byValInfo for PutDirect on a typed array as OOB.
However, this won't matter in terms of perf impact. We would never optimize this
to anything smart in the JIT since we don't model PutDirect to typed arrays in
the JIT. (I should actually add an assertion for this in the DFG backend and FTLLower.)

[Geoffrey Garen]

Comment on attachment 309017 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309017&action=review

r=me

> Source/JavaScriptCore/bytecode/ByValInfo.h:180
> +    default:
> +        return false;

This default includes JITDirectArguments and JITScopedArguments, but your comment only covers typed arrays. Was it intentional to include JITDirectArguments and JITScopedArguments in forbidding PutDirect? If so, I would mention that, and why.

[Saam Barati]

Created attachment 309184 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 309184 [details]
patch for landing

Clearing flags on attachment: 309184

Committed r216279: <http://trac.webkit.org/changeset/216279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 309184 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=309184&action=review

> JSTests/stress/put-direct-index-broken-2.js:247
> +function assert(b) {
> +    if (!b)
> +        throw new Error("Bad assertion");
> +}
> +
> +function test(f) {
> +    for (let i = 0; i < 1000; ++i) {
> +        f();
> +    }
> +}
> +
> +let __oldDesc = null;
> +let __localLength;
> +function makeLengthWritable() {
> +    assert(__oldDesc === null);
> +    __oldDesc = Object.getOwnPropertyDescriptor(Uint8ClampedArray.prototype.__proto__, "length");
> +    assert(typeof __oldDesc.get === "function");
> +    Reflect.defineProperty(Uint8ClampedArray.prototype.__proto__, "length", {configurable:true, get() { return __localLength; }, set(x) { __localLength = x; }});
> +}
> +
> +function restoreOldDesc() {
> +    assert(__oldDesc !== null);
> +    Reflect.defineProperty(Uint8ClampedArray.prototype.__proto__, "length", __oldDesc);
> +    __oldDesc = null;
> +}
> +
> +test(function() {
> +    "use strict";
> +    let a = [];
> +    a.push(300);
> +    a.length = 4277;
> +
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +    let err = null;
> +    try {
> +        let y = Array.prototype.map.call(a, x => x);
> +    } catch(e) {
> +        err = e;
> +    }
> +    assert(!!err);
> +    assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0");
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +    let err = null;
> +    try {
> +        let y = Array.prototype.filter.call(a, x => true);
> +    } catch(e) {
> +        err = e;
> +    }
> +    assert(err.toString() ===  "TypeError: Attempting to configure non-configurable property on a typed array at index: 0");
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +    let err = null;
> +    let y = Array.prototype.filter.call(a, x => false);
> +    assert(y instanceof Uint8ClampedArray);
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +
> +    let err = null;
> +    try {
> +        let y = Array.prototype.slice.call(a, 0);
> +    } catch(e) {
> +        err = e;
> +    }
> +    assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0");
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +
> +    makeLengthWritable();
> +    let y = Array.prototype.slice.call(a, 100);
> +    assert(y.length === 4277 - 100);
> +    assert(y.length === __localLength);
> +    assert(y instanceof Uint8ClampedArray);
> +    restoreOldDesc();
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +
> +    makeLengthWritable();
> +    let y = Array.prototype.splice.call(a);
> +    assert(y.length === __localLength);
> +    assert(y.length === 0);
> +    restoreOldDesc();
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +
> +    let err = null;
> +    try {
> +        let y = Array.prototype.splice.call(a, 0);
> +    } catch(e) {
> +        err = e;
> +    }
> +    assert(err.toString() === "TypeError: Attempting to configure non-configurable property on a typed array at index: 0");
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let x = new Uint8ClampedArray;
> +    a.__proto__ = x;
> +
> +    makeLengthWritable();
> +    let y = Array.prototype.slice.call(a, 100);
> +    assert(y.length === 4277 - 100);
> +    assert(y.length === __localLength);
> +    assert(y instanceof Uint8ClampedArray);
> +    restoreOldDesc();
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let calls = 0;
> +    let target = {};
> +    a.__proto__ = {
> +        constructor: {
> +            [Symbol.species]: function(length) {
> +                assert(length === 4277)
> +                return new Proxy(target, {
> +                    defineProperty(...args) {
> +                        ++calls;
> +                        return Reflect.defineProperty(...args);
> +                    }
> +                });
> +            }
> +        }
> +    };
> +    let y = Array.prototype.map.call(a, x => x);
> +    assert(calls === 100);
> +    for (let i = 0; i < 100; ++i)
> +        assert(target[i] === i);
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let calls = 0;
> +    let target = {};
> +    a.__proto__ = {
> +        constructor: {
> +            [Symbol.species]: function(length) {
> +                assert(length === 0)
> +                return new Proxy(target, {
> +                    defineProperty(...args) {
> +                        ++calls;
> +                        return Reflect.defineProperty(...args);
> +                    }
> +                });
> +            }
> +        }
> +    };
> +    let y = Array.prototype.filter.call(a, x => true);
> +    assert(calls === 100);
> +    for (let i = 0; i < 100; ++i)
> +        assert(target[i] === i);
> +});
> +
> +test(function() {
> +    let a = [];
> +    for (let i = 0; i < 100; i++) {
> +        a.push(i);
> +    }
> +    a.length = 4277;
> +    let calls = 0;
> +    let target = {};
> +    let keys = [];
> +    a.__proto__ = {
> +        constructor: {
> +            [Symbol.species]: function(length) {
> +                assert(length === 4277)
> +                return new Proxy(target, {
> +                    defineProperty(...args) {
> +                        keys.push(args[1])
> +                        ++calls;
> +                        return Reflect.defineProperty(...args);
> +                    }
> +                });
> +            }
> +        }
> +    };
> +    let y = Array.prototype.slice.call(a, 0);
> +    assert(calls === 101); // length gets defined too.
> +    assert(keys.length === 101);
> +    for (let i = 0; i < 100; ++i) {
> +        assert(parseInt(keys[i]) === i);
> +        assert(target[i] === i);
> +    }
> +    assert(keys[keys.length - 1] === "length");
> +    assert(target.length === 4277);
> +});

This test is super slow.  Is there a way to write such tests so that they aren't super slow?

For example: maybe instead of having each test simply run 1000 times, have just the part of the test that needs to warm up run 1000 times.