The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell` if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things: 1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by `CodeBlock::getOrAddArrayProfile`. 2) get_by_id's mode *is* ArrayLengt and we generate the array profiling code pointing to the ArrayProfile that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens, the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.
<rdar://problem/23453006>
Created attachment 358193 [details] Patch
Comment on attachment 358193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358193&action=review > Source/JavaScriptCore/ChangeLog:16 > + The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell` > + if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry > + in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things: > + 1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by > + `CodeBlock::getOrAddArrayProfile`. > + 2) get_by_id's mode *is* ArrayLength and we generate the array profiling code pointing to the ArrayProfile > + that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens, > + the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode. I don't understand: I see that metadata.mode is set to GetByIdMode::ArrayLength in the LLInt slow_path_get_by_id. From reading the code, the determination of metadata.mode is a function of the baseValue and the PropertySlot returned by get(). This means that metadata.mode is set at runtime, and can vary depending on runtime variables. What's keeping it from changing again after it has been set once? Your baseline code seems to be making an assumption about where to get the ArrayProfile address from. I see this in emitArrayProfilingSiteForBytecodeIndexWithCell() which gets the ArrayProfile address from m_codeBlock->getOrAddArrayProfile(), which in turn determines the ArrayProfile address based on metadata.mode. Since metadata.mode can change at runtime, the baseline JIT will be racing against the mutator thread. The baseline JIT may compile with a GetByIdMode::ArrayLength type ArrayProfile and the mutator thread may change the metadata.mode to something other than GetByIdMode::ArrayLength right after, or vice versa. The baseline JIT emitted runtime check (that you added below) comparing metadata.mode against GetByIdMode::ArrayLength will not protect against this. Is there a reason why the GetById metadata can't have a static shape? I think that that would be the ideal solution.
Comment on attachment 358193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358193&action=review >> Source/JavaScriptCore/ChangeLog:16 >> + the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode. > > I don't understand: I see that metadata.mode is set to GetByIdMode::ArrayLength in the LLInt slow_path_get_by_id. From reading the code, the determination of metadata.mode is a function of the baseValue and the PropertySlot returned by get(). This means that metadata.mode is set at runtime, and can vary depending on runtime variables. What's keeping it from changing again after it has been set once? > > Your baseline code seems to be making an assumption about where to get the ArrayProfile address from. I see this in emitArrayProfilingSiteForBytecodeIndexWithCell() which gets the ArrayProfile address from m_codeBlock->getOrAddArrayProfile(), which in turn determines the ArrayProfile address based on metadata.mode. Since metadata.mode can change at runtime, the baseline JIT will be racing against the mutator thread. The baseline JIT may compile with a GetByIdMode::ArrayLength type ArrayProfile and the mutator thread may change the metadata.mode to something other than GetByIdMode::ArrayLength right after, or vice versa. > > The baseline JIT emitted runtime check (that you added below) comparing metadata.mode against GetByIdMode::ArrayLength will not protect against this. Is there a reason why the GetById metadata can't have a static shape? I think that that would be the ideal solution. I could move the ArrayProfile out of the mode specific metadata, but that would add an extra 16 bytes to the get_by_id metadata, which would only be used for the ArrayLength mode... this seems a bit wasteful. I think I'd rather just use `emitArrayProfilingSiteWithCell` from `emit_op_get_by_id` and make sure we always refer to the ArrayProfile in the metadata. (And perhaps replace the conditional return in the `CodeBlock::getArrayProfile` with an assertion, not sure if that's correct though.) Do you think that makes sense?
Created attachment 358202 [details] Patch
Created attachment 358213 [details] Patch
Comment on attachment 358213 [details] Patch Attachment 358213 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10609586 New failing tests: stress/v8-splay-strict.js.dfg-eager v8-v6/v8-splay.js.dfg-eager apiTests
Comment on attachment 358213 [details] Patch Attachment 358213 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10609895 New failing tests: js/dfg-inline-arguments-use-from-all-the-places-broken.html
Created attachment 358225 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358213 [details] Patch Attachment 358213 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10609708 New failing tests: js/dfg-inline-arguments-use-from-all-the-places-broken.html
Created attachment 358228 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 358213 [details] Patch Attachment 358213 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10610619 New failing tests: imported/w3c/web-platform-tests/IndexedDB/interleaved-cursors-small.html
Created attachment 358239 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 358251 [details] Patch
Comment on attachment 358251 [details] Patch r=me
Comment on attachment 358251 [details] Patch Clearing flags on attachment: 358251 Committed r239626: <https://trac.webkit.org/changeset/239626>
All reviewed patches have been landed. Closing bug.