Created attachment 387942 [details] Patch

Comment on attachment 387942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387942&action=review Looks really nice. I would like to have StringObject handling, which is implemented in ToPrimitive too. Then looks perfect. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1394 > + case ToPropertyKey: { Let's handle StringObject case as it is done in ToPrimitive (and add tests). If we detect the input is StringObject and m_graph.canOptimizeStringObjectAccess is true, let's insert a check, and convert it to ToString<>. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:583 > + } Let's handle StringObject case too, as it is done in ToPrimitive case.

Comment on attachment 387942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387942&action=review LGTM too. I found one minor perf bug > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2530 > + m_state.setShouldTryConstantFolding(true); You need to write the code inside DFGConstantFoldingPhase that corresponds to this and actually does the identity conversion.

Created attachment 388049 [details] Patch v2

Comment on attachment 387942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387942&action=review Thanks very much for the review. I'd like Caio to get the chance to review this as well, and there is probably some room for improvement before I set CQ+ >> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2530 >> + m_state.setShouldTryConstantFolding(true); > > You need to write the code inside DFGConstantFoldingPhase that corresponds to this and actually does the identity conversion. Done >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1394 >> + case ToPropertyKey: { > > Let's handle StringObject case as it is done in ToPrimitive (and add tests). > If we detect the input is StringObject and m_graph.canOptimizeStringObjectAccess is true, let's insert a check, and convert it to ToString<>. Done, but I'm not super confident on it, so please take another look >> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:583 >> + } > > Let's handle StringObject case too, as it is done in ToPrimitive case. Done (but again, please take another look to make sure the logic I came up with is sound)

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review Sorry for the delay here. I just have one minor comment about Prediction propagation. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:580 > + if (child) I think we should be able to predict `SpecString | SpecSymbol` even when `child == SpecNone`, given those are the only possible output for `ToPropertyKey`.

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review >> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:580 >> + if (child) > > I think we should be able to predict `SpecString | SpecSymbol` even when `child == SpecNone`, given those are the only possible output for `ToPropertyKey`. No. Prediction propagation phase is iterative. So, getting SpecNone here is not meaning that this is the final result (let's consider loops etc.). Returning `SpecString | SpecSymbol` for SpecNone pollutes type prediction information unnecessarily and produces inefficient generic code. And, we should return SpecNone for SpecNone, this is better. This phase is not AbstractInterpreter: AI needs to offer proven all possible type, on the other hand, prediction propagation needs to offer "prediction" for efficient execution. The input = SpecNone typically means this code is never executed. Even though, this type prediction could be merged into the active path later (due to the control flow), and in that case, we pollute the prediction accidentally, and leading to the inefficient code.

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review r=me > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1412 > + if (node->child1()->shouldSpeculateStringObject() > + && m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) { > + addCheckStructureForOriginalStringObjectUse(StringObjectUse, node->origin, node->child1().node()); > + fixEdge<StringObjectUse>(node->child1()); > + node->convertToToString(); > + return; Looks nice. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1414 > + break; You can also add `node->child1()->shouldSpeculateStringOrStringObject()` case as ToPrimitive is doing. > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1449 > + if (type & SpecStringObject && m_graph.canOptimizeStringObjectAccess(m_currentNode->origin.semantic)) > + return mergeSpeculations(type & SpecSymbol, SpecString); Nice.

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review >>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:580 >>> + if (child) >> >> I think we should be able to predict `SpecString | SpecSymbol` even when `child == SpecNone`, given those are the only possible output for `ToPropertyKey`. > > No. Prediction propagation phase is iterative. So, getting SpecNone here is not meaning that this is the final result (let's consider loops etc.). Returning `SpecString | SpecSymbol` for SpecNone pollutes type prediction information unnecessarily and produces inefficient generic code. > And, we should return SpecNone for SpecNone, this is better. This phase is not AbstractInterpreter: AI needs to offer proven all possible type, on the other hand, prediction propagation needs to offer "prediction" for efficient execution. > The input = SpecNone typically means this code is never executed. Even though, this type prediction could be merged into the active path later (due to the control flow), and in that case, we pollute the prediction accidentally, and leading to the inefficient code. One way to consider about this phase is, "Given this prediction input, what is the minimal possible type prediction which covers the input prediction's use case?" So, if the prediction input is saying SpecNone, SpecNone is the minimal possible type prediction output which covers the code producing the input's prediction.

(In reply to Yusuke Suzuki from comment #9) > Comment on attachment 388049 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388049&action=review > > >>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:580 > >>> + if (child) > >> > >> I think we should be able to predict `SpecString | SpecSymbol` even when `child == SpecNone`, given those are the only possible output for `ToPropertyKey`. > > > > No. Prediction propagation phase is iterative. So, getting SpecNone here is not meaning that this is the final result (let's consider loops etc.). Returning `SpecString | SpecSymbol` for SpecNone pollutes type prediction information unnecessarily and produces inefficient generic code. > > And, we should return SpecNone for SpecNone, this is better. This phase is not AbstractInterpreter: AI needs to offer proven all possible type, on the other hand, prediction propagation needs to offer "prediction" for efficient execution. > > The input = SpecNone typically means this code is never executed. Even though, this type prediction could be merged into the active path later (due to the control flow), and in that case, we pollute the prediction accidentally, and leading to the inefficient code. > > One way to consider about this phase is, "Given this prediction input, what > is the minimal possible type prediction which covers the input prediction's > use case?" > So, if the prediction input is saying SpecNone, SpecNone is the minimal > possible type prediction output which covers the code producing the input's > prediction. Right! Thanks for the clarification.

Created attachment 388103 [details] Patch for landing

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1414 >> + break; > > You can also add `node->child1()->shouldSpeculateStringOrStringObject()` case as ToPrimitive is doing. Done in the patch for landing. I've added a test, which I hope exercises this path, but hard to be sure. >>>>> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:580 >>>>> + if (child) >>>> >>>> I think we should be able to predict `SpecString | SpecSymbol` even when `child == SpecNone`, given those are the only possible output for `ToPropertyKey`. >>> >>> No. Prediction propagation phase is iterative. So, getting SpecNone here is not meaning that this is the final result (let's consider loops etc.). Returning `SpecString | SpecSymbol` for SpecNone pollutes type prediction information unnecessarily and produces inefficient generic code. >>> And, we should return SpecNone for SpecNone, this is better. This phase is not AbstractInterpreter: AI needs to offer proven all possible type, on the other hand, prediction propagation needs to offer "prediction" for efficient execution. >>> The input = SpecNone typically means this code is never executed. Even though, this type prediction could be merged into the active path later (due to the control flow), and in that case, we pollute the prediction accidentally, and leading to the inefficient code. >> >> One way to consider about this phase is, "Given this prediction input, what is the minimal possible type prediction which covers the input prediction's use case?" >> So, if the prediction input is saying SpecNone, SpecNone is the minimal possible type prediction output which covers the code producing the input's prediction. > > Right! Thanks for the clarification. +1, thanks a lot for the explanation here.

Comment on attachment 388049 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=388049&action=review >>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1414 >>> + break; >> >> You can also add `node->child1()->shouldSpeculateStringOrStringObject()` case as ToPrimitive is doing. > > Done in the patch for landing. I've added a test, which I hope exercises this path, but hard to be sure. well, I've just now verified that the path is taken in the test, so that's promising. This doesn't seem like something that would likely be hit in practice, but I guess it's not too expensive to support it. ¯\(°_o)/¯

I flipped cq+ last night, and reflipped today, but doesn’t seem to be going anywhere

Comment on attachment 388103 [details] Patch for landing Reflipped.

cq is currently in migration state from High Sierra => Mojave IIUC. And maybe, in some trouble, not sure whether this is fixed already. Try refliping again now.

The commit-queue encountered the following flaky tests while processing attachment 388103 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 388103 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 388103 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 388103 [details] Patch for landing Clearing flags on attachment: 388103 Committed r254801: <https://trac.webkit.org/changeset/254801>

All reviewed patches have been landed. Closing bug.

<rdar://problem/58713990>