Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479123002:
Compiling an array literal should be context-independent. (Closed)
DescriptionCompiling an array literal should be context-independent.
We are removing use of the debugger context. When the debugger triggers
compilation, we may not have a context from which to create a JSArray.
R=ishell@chromium.org
BUG=chromium:664577
Committed: https://crrev.com/08f09ed71be6d50dbc73f50acee1e32d14b8a947
Cr-Commit-Position: refs/heads/master@{#40956}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebase and address comment #
Total comments: 6
Patch Set 3 : address comment #Patch Set 4 : address comments #Patch Set 5 : rebase #
Messages
Total messages: 39 (20 generated)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a nit https://codereview.chromium.org/2479123002/diff/1/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/2479123002/diff/1/src/ast/ast.cc#newcode631 src/ast/ast.cc:631: if (IsDoubleOrFloatElementsKind(kind)) { Maybe IsFastDoubleElementsKind(kind)? We don't involve typed array elements kinds during elements transitions.
yangguo@chromium.org changed reviewers: + verwaest@chromium.org
Toon, could you take a look as owner of src/ast?
That's funny :) Some comments. https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode607 src/ast/ast.cc:607: is_holey = true; if you already do kind = GetHoleyElementsKind(kind) here, you can drop the is_holey flag and additional branch below. https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode641 src/ast/ast.cc:641: fixed_double_array->set(i, element->Number()); What about having a CopyElements on the elements accessor that doesn't take a JSObject* from_holder but FixedArrayBase* from, that way we can use the regular CopySmiToDoubleElements here. It would look like if (IsFastDoubleElementsKind(kind)) { ElementsAccessor* accessor = ElementsAccessor::ForKind(kind); elements = isolate->factory()->NewFixedDoubleArray(constants_length); accessor->CopyElements(fixed_array, elements); }
https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode583 src/ast/ast.cc:583: ElementsKind kind = FAST_SMI_ELEMENTS; FIRST_FAST_ELEMENTS_KIND?
Addressed comment. Please take another look.
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
There were other comments than the last mail :)
https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc File src/ast/ast.cc (right): https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode583 src/ast/ast.cc:583: ElementsKind kind = FAST_SMI_ELEMENTS; On 2016/11/08 10:18:02, Toon Verwaest wrote: > FIRST_FAST_ELEMENTS_KIND? Done. https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode607 src/ast/ast.cc:607: is_holey = true; On 2016/11/08 10:16:54, Toon Verwaest wrote: > if you already do kind = GetHoleyElementsKind(kind) here, you can drop the > is_holey flag and additional branch below. Unfortunately this does not work due to how GetMoreGeneralElementsKind works. We could go from FAST_SMI_ELEMENTS to FAST_HOLEY_SMI_ELEMENTS, and then upon seeing a double, go to FAST_DOUBLE_ELEMENTS, but we should have gone to FAST_HOLEY_DOUBLE_ELEMENTS. Is this something that needs to be addressed elsewhere? https://codereview.chromium.org/2479123002/diff/20001/src/ast/ast.cc#newcode641 src/ast/ast.cc:641: fixed_double_array->set(i, element->Number()); On 2016/11/08 10:16:54, Toon Verwaest wrote: > What about having a CopyElements on the elements accessor that doesn't take a > JSObject* from_holder but FixedArrayBase* from, that way we can use the regular > CopySmiToDoubleElements here. It would look like > > if (IsFastDoubleElementsKind(kind)) { > ElementsAccessor* accessor = ElementsAccessor::ForKind(kind); > elements = isolate->factory()->NewFixedDoubleArray(constants_length); > accessor->CopyElements(fixed_array, elements); > } Done.
lgtm, thanks
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2479123002/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
The CQ bit was checked by yangguo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org, verwaest@chromium.org Link to the patchset: https://codereview.chromium.org/2479123002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2481363009/ by yangguo@chromium.org. The reason for reverting is: speculative revert to fix https://uberchromegw.corp.google.com/i/client.v8/builders/V8%20Mac%20GC%20Str....
Message was sent while issue was closed.
Description was changed from ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org ========== to ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org ==========
The CQ bit was checked by yangguo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Relanding this. The debug-scopes flakiness did not go away by reverting this.
Description was changed from ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org ========== to ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org BUG=chromium:664577 ==========
Message was sent while issue was closed.
Description was changed from ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org BUG=chromium:664577 ========== to ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org BUG=chromium:664577 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org BUG=chromium:664577 ========== to ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org Committed: https://crrev.com/e5c1929c0937c021b0bb8d9b237990bead000a10 Cr-Commit-Position: refs/heads/master@{#40884} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e5c1929c0937c021b0bb8d9b237990bead000a10 Cr-Commit-Position: refs/heads/master@{#40884}
Message was sent while issue was closed.
Description was changed from ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org Committed: https://crrev.com/e5c1929c0937c021b0bb8d9b237990bead000a10 Cr-Commit-Position: refs/heads/master@{#40884} ========== to ========== Compiling an array literal should be context-independent. We are removing use of the debugger context. When the debugger triggers compilation, we may not have a context from which to create a JSArray. R=ishell@chromium.org BUG=chromium:664577 Committed: https://crrev.com/08f09ed71be6d50dbc73f50acee1e32d14b8a947 Cr-Commit-Position: refs/heads/master@{#40956} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/08f09ed71be6d50dbc73f50acee1e32d14b8a947 Cr-Commit-Position: refs/heads/master@{#40956} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
