JSObjects should have a mask for loading indexed properties
Created attachment 329259 [details] Patch
Created attachment 329263 [details] Patch
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.
Created attachment 329272 [details] Patch
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.
Created attachment 329277 [details] Patch
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.
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.
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"
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.
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
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
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.
Created attachment 329360 [details] Patch for landing
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
Committed r225913: <https://trac.webkit.org/changeset/225913>
<rdar://problem/36052908>
Looks like this broke a bunch of test262 tests. Investigating...
It seems that this introduce performance regression in Kraken, Octane, and ARES-6 by 5.4%, 2.3%, and 1.8% respectively.
It looks like this introduces segfaults in many tests on 32 bit platforms as well.
(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)
(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.
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?
(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.