Bug 180768 – JSObjects should have a mask for loading indexed properties

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

JSObjects should have a mask for loading indexed properties

[Keith Miller]

Created attachment 329259 [details]
Patch

[Keith Miller]

Created attachment 329263 [details]
Patch

[EWS Watchlist]

Attachment 329263 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeap.h:136:  The parameter name "out" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 329272 [details]
Patch

[EWS Watchlist]

Attachment 329272 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeap.h:136:  The parameter name "out" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 329277 [details]
Patch

[EWS Watchlist]

Attachment 329277 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeap.h:136:  The parameter name "out" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 329277 [details]
Patch

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

> Source/JavaScriptCore/runtime/Butterfly.h:45
> -        : m_data(0)
> +        : m_data(nullptr)
>  #if !ASSERT_DISABLED
>          , m_length(0)
>  #endif

I suggest just initializing these in the declaration of m_data and m_length below using { } notation instead.

[Keith Miller]

Comment on attachment 329277 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Butterfly.h:45
>>  #endif
> 
> I suggest just initializing these in the declaration of m_data and m_length below using { } notation instead.

Sure. I changed it locally and made this into "ContiguousData() = default"

[Mark Lam]

Comment on attachment 329277 [details]
Patch

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

r=me with fixes.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:1065
> +            // If we were to have any indexed properties then we would need update the indexing mask on the base object.

did you mean "would need to update"?  I also suggest putting a comma after "indexed properties" before "then".

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7701
> +        // X86 only has 6 GP registers, which is not enough for the fast case here. At least without custom code, which is not currently worth the extra code mantainence.

/mantainence/maintenance/.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1377
> +    // sizeGPR may be either scratch register. If for whatever reason the butterfly is going to change vector length this function does NOT
> +    // update the indexing mask.

This comment seems out of place.  There's no use of sizeGPR in the function below.

> Source/JavaScriptCore/runtime/Butterfly.h:135
> +    // If vectorLength() is zero clz is undefined.
> +    uint32_t computeIndexingMask() { return computeIndexingMaskForVectorLength(vectorLength()); } // vectorLength() ? ~((1ll << 63) >> std::clz(static_cast<uint64_t>(vectorLength()) * sizeof(EncodedJSValue))) : 0; }

The comment at line 134 says that clz is undefined for a zero vectorLength, but then you go ahead and just call computeIndexingMaskForVectorLength() which uses std::clz() without a zero check first.
The comment after the function at line 135 doesn't seem to describe what this function does.  It says that the return value is scaled by sizeof(EncodedJSValue).  I think that is wrong.

Please fix.

> Source/JavaScriptCore/runtime/JSObject.cpp:2611
> +    auto oldButterfly = butterfly;
> +    UNUSED_PARAM(oldButterfly);

What is this for?  Please remove if not needed.

[EWS Watchlist]

Comment on attachment 329277 [details]
Patch

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

New failing tests:
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-array-buffer-view.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/rsa.worker.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html
imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/test_ecdsa.https.html
imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/ecdsa.worker.html
webgl/1.0.2/conformance/textures/texture-hd-dpi.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_rsa_oaep.https.html
webgl/1.0.2/conformance/textures/texture-size-cube-maps.html
imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/rsa_pkcs.worker.html
imported/w3c/web-platform-tests/streams/readable-byte-streams/general.dedicatedworker.html
imported/w3c/web-platform-tests/WebCryptoAPI/sign_verify/test_rsa_pkcs.https.html
imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.https.html
webgl/1.0.2/conformance/extensions/webgl-compressed-texture-s3tc.html
fast/canvas/webgl/drawingbuffer-test.html

[EWS Watchlist]

Created attachment 329300 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

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

[Keith Miller]

Comment on attachment 329277 [details]
Patch

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

I also forgot to add a new test for checking flattening structures where we end up removing the butterfly. AFAICT we didn't have test coverage for this before.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:1065
>> +            // If we were to have any indexed properties then we would need update the indexing mask on the base object.
> 
> did you mean "would need to update"?  I also suggest putting a comma after "indexed properties" before "then".

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7701
>> +        // X86 only has 6 GP registers, which is not enough for the fast case here. At least without custom code, which is not currently worth the extra code mantainence.
> 
> /mantainence/maintenance/.

Fixed.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:1377
>> +    // update the indexing mask.
> 
> This comment seems out of place.  There's no use of sizeGPR in the function below.

Hmm, yeah I'll get rid of the first sentence.

>> Source/JavaScriptCore/runtime/Butterfly.h:135
>> +    uint32_t computeIndexingMask() { return computeIndexingMaskForVectorLength(vectorLength()); } // vectorLength() ? ~((1ll << 63) >> std::clz(static_cast<uint64_t>(vectorLength()) * sizeof(EncodedJSValue))) : 0; }
> 
> The comment at line 134 says that clz is undefined for a zero vectorLength, but then you go ahead and just call computeIndexingMaskForVectorLength() which uses std::clz() without a zero check first.
> The comment after the function at line 135 doesn't seem to describe what this function does.  It says that the return value is scaled by sizeof(EncodedJSValue).  I think that is wrong.
> 
> Please fix.

Whoops. I thought I deleted this earlier. I guess I accidentally reverted that change locally.

>> Source/JavaScriptCore/runtime/JSObject.cpp:2611
>> +    UNUSED_PARAM(oldButterfly);
> 
> What is this for?  Please remove if not needed.

Removed.

[Keith Miller]

Created attachment 329360 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 329360 [details]
Patch for landing

Rejecting attachment 329360 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 329360, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/Structure.cpp
patching file Source/WTF/wtf/StdLibExtras.h
Hunk #1 succeeded at 548 (offset 8 lines).
patching file Source/WebCore/bindings/js/JSDOMConvertSequences.h
patching file JSTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 1.
patching file JSTests/stress/flattening-prototype-remove-butterfly.js
patching file JSTests/stress/int16-put-by-val-in-and-out-of-bounds.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/5660968

[Keith Miller]

Committed r225913: <https://trac.webkit.org/changeset/225913>

[Radar WebKit Bug Importer]

<rdar://problem/36052908>

[Keith Miller]

Looks like this broke a bunch of test262 tests. Investigating...

[Yusuke Suzuki]

It seems that this introduce performance regression in Kraken, Octane, and ARES-6 by 5.4%, 2.3%, and 1.8% respectively.

[Guillaume Emont]

It looks like this introduces segfaults in many tests on 32 bit platforms as well.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #20)
> It looks like this introduces segfaults in many tests on 32 bit platforms as
> well.

Had a look at one: stress/const-tdz.js

When compiling in debug, I get this assert:

ASSERTION FAILED: numberOfFrames >= framesToSkip
../../Source/WTF/wtf/StackTrace.cpp(77) : static std::unique_ptr<WTF::StackTrace> WTF::StackTrace::captureStackTrace(int, int)

with backtrace:

#0  0x2d069940 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:272
#1  0x2d0c7da0 in WTF::StackTrace::captureStackTrace (maxFrames=100, framesToSkip=2) at ../../Source/WTF/wtf/StackTrace.cpp:77
#2  0x2ced7a9c in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8, exception=0x325a4280) at ../../Source/JavaScriptCore/runtime/VM.cpp:702
#3  0x2ced7bdc in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8, thrownValue=...) at ../../Source/JavaScriptCore/runtime/VM.cpp:714
#4  0x2ced7c94 in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8, error=0x325a4250) at ../../Source/JavaScriptCore/runtime/VM.cpp:720
#5  0x2ceab26c in JSC::ThrowScope::throwException (this=0x7fff6144, exec=0x7fff61c8, obj=0x325a4250) at ../../Source/JavaScriptCore/runtime/ThrowScope.cpp:94
#6  0x00472558 in JSC::throwException (exec=0x7fff61c8, scope=..., obj=0x325a4250) at ../../Source/JavaScriptCore/runtime/ThrowScope.h:104
#7  0x2cac5a9c in JSC::slow_path_throw_tdz_error (exec=0x7fff61c8, pc=0x302f5b20) at ../../Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:276
#8  0x2c85f958 in llint_entry () at ../../Source/JavaScriptCore/runtime/ExceptionScope.h:46

(on MIPS)

[Guillaume Emont]

(In reply to Guillaume Emont from comment #21)
> (In reply to Guillaume Emont from comment #20)
> > It looks like this introduces segfaults in many tests on 32 bit platforms as
> > well.
> 
> Had a look at one: stress/const-tdz.js
> 
> When compiling in debug, I get this assert:
> 
> ASSERTION FAILED: numberOfFrames >= framesToSkip
> ../../Source/WTF/wtf/StackTrace.cpp(77) : static
> std::unique_ptr<WTF::StackTrace> WTF::StackTrace::captureStackTrace(int, int)
> 
> with backtrace:
> 
> #0  0x2d069940 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:272
> #1  0x2d0c7da0 in WTF::StackTrace::captureStackTrace (maxFrames=100,
> framesToSkip=2) at ../../Source/WTF/wtf/StackTrace.cpp:77
> #2  0x2ced7a9c in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8,
> exception=0x325a4280) at ../../Source/JavaScriptCore/runtime/VM.cpp:702
> #3  0x2ced7bdc in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8,
> thrownValue=...) at ../../Source/JavaScriptCore/runtime/VM.cpp:714
> #4  0x2ced7c94 in JSC::VM::throwException (this=0x324f5000, exec=0x7fff61c8,
> error=0x325a4250) at ../../Source/JavaScriptCore/runtime/VM.cpp:720
> #5  0x2ceab26c in JSC::ThrowScope::throwException (this=0x7fff6144,
> exec=0x7fff61c8, obj=0x325a4250) at
> ../../Source/JavaScriptCore/runtime/ThrowScope.cpp:94
> #6  0x00472558 in JSC::throwException (exec=0x7fff61c8, scope=...,
> obj=0x325a4250) at ../../Source/JavaScriptCore/runtime/ThrowScope.h:104
> #7  0x2cac5a9c in JSC::slow_path_throw_tdz_error (exec=0x7fff61c8,
> pc=0x302f5b20) at ../../Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:276
> #8  0x2c85f958 in llint_entry () at
> ../../Source/JavaScriptCore/runtime/ExceptionScope.h:46
> 
> (on MIPS)

For the record, I forgot what it was, but this got fixed a few days later and is not an issue any more.

[Guillaume Emont]

Comment on attachment 329360 [details]
Patch for landing

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2772
> +                m_jit.and32(MacroAssembler::Address(baseReg, JSObject::butterflyIndexingMaskOffset()), propertyReg);

These changes for Array::Int32, Array::Contiguous and Array::Double are not mirrored in DFGSPeculativeJIT32_64.cpp. Is there a reason for that or was that forgotten?

[Keith Miller]

(In reply to Guillaume Emont from comment #23)
> Comment on attachment 329360 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329360&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2772
> > +                m_jit.and32(MacroAssembler::Address(baseReg, JSObject::butterflyIndexingMaskOffset()), propertyReg);
> 
> These changes for Array::Int32, Array::Contiguous and Array::Double are not
> mirrored in DFGSPeculativeJIT32_64.cpp. Is there a reason for that or was
> that forgotten?

Yeah, this was an oversight. I filed https://bugs.webkit.org/show_bug.cgi?id=183411 to fix it.