Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582583002:
[wasm] simpler detection if we compiled asm-wasm (Closed)
|
Created:
4 years ago by Mircea Trofin Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] simpler detection if we compiled asm-wasm
BUG=643595
Review-Url: https://codereview.chromium.org/2582583002
Cr-Original-Commit-Position: refs/heads/master@{#41738}
Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec54edd
Review-Url: https://codereview.chromium.org/2582583002
Cr-Commit-Position: refs/heads/master@{#41743}
Committed: https://chromium.googlesource.com/v8/v8/+/93e53da4c846327760c412227a6a5983c9ca86fb
Patch Set 1 #Patch Set 2 : using new iterator #Messages
Total messages: 30 (18 generated)
The CQ bit was checked by mtrofin@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...
Description was changed from ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 ========== to ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, yangguo@chromium.org
ptal - thanks, Yang, for the suggestion, this looks simpler!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/15 16:51:19, Mircea Trofin wrote: > ptal - thanks, Yang, for the suggestion, this looks simpler! lgtm
The CQ bit was checked by mtrofin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1481829383071420, "parent_rev":
"03f33f2e68641903a982a025b0ba645c8ba9f9b3", "commit_rev":
"cb433bed0b940a7073b5215e6684b940eec54edd"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 ========== to ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 Review-Url: https://codereview.chromium.org/2582583002 Cr-Commit-Position: refs/heads/master@{#41738} Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec...
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2582623002/ by mtrofin@chromium.org. The reason for reverting is: https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds....
Message was sent while issue was closed.
On 2016/12/15 19:35:49, Mircea Trofin wrote: > A revert of this CL (patchset #1 id:1) has been created in > https://codereview.chromium.org/2582623002/ by mailto:mtrofin@chromium.org. > > The reason for reverting is: > https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds.... Oh yeah I think you need to check upfront whether it's a weak fixed array at all. Iirc it's not if there are no shared function infos.
Message was sent while issue was closed.
On 2016/12/15 19:49:15, Yang wrote: > On 2016/12/15 19:35:49, Mircea Trofin wrote: > > A revert of this CL (patchset #1 id:1) has been created in > > https://codereview.chromium.org/2582623002/ by mailto:mtrofin@chromium.org. > > > > The reason for reverting is: > > > https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds.... > > Oh yeah I think you need to check upfront whether it's a weak fixed array at > all. Iirc it's not if there are no shared function infos. Would it be more usable if the check happened in the iterator, or, even better (imho) if the iterator were appropriately typed? Currently, the constructor accepts an Object*
Message was sent while issue was closed.
On 2016/12/15 19:51:09, Mircea Trofin wrote: > On 2016/12/15 19:49:15, Yang wrote: > > On 2016/12/15 19:35:49, Mircea Trofin wrote: > > > A revert of this CL (patchset #1 id:1) has been created in > > > https://codereview.chromium.org/2582623002/ by mailto:mtrofin@chromium.org. > > > > > > The reason for reverting is: > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds.... > > > > Oh yeah I think you need to check upfront whether it's a weak fixed array at > > all. Iirc it's not if there are no shared function infos. > > Would it be more usable if the check happened in the iterator, or, even better > (imho) if the iterator were appropriately typed? Currently, the constructor > accepts an Object* Yeah that probably helps. But all of this changed here anyways: https://codereview.chromium.org/2577063002/diff/20001/src/objects.h
Message was sent while issue was closed.
On 2016/12/15 19:56:03, Yang wrote: > On 2016/12/15 19:51:09, Mircea Trofin wrote: > > On 2016/12/15 19:49:15, Yang wrote: > > > On 2016/12/15 19:35:49, Mircea Trofin wrote: > > > > A revert of this CL (patchset #1 id:1) has been created in > > > > https://codereview.chromium.org/2582623002/ by > mailto:mtrofin@chromium.org. > > > > > > > > The reason for reverting is: > > > > > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds.... > > > > > > Oh yeah I think you need to check upfront whether it's a weak fixed array at > > > all. Iirc it's not if there are no shared function infos. > > > > Would it be more usable if the check happened in the iterator, or, even better > > (imho) if the iterator were appropriately typed? Currently, the constructor > > accepts an Object* > > Yeah that probably helps. But all of this changed here anyways: > https://codereview.chromium.org/2577063002/diff/20001/src/objects.h It's more involved. It seems that there's no way to tell if an object is FixedArray or WeakFixedArray. Then, the semantics of "get" and "set" in a WeakFixedArray are different from the normal FixedArray, because we use the first index to store the last accessed index. The situation seems fragile. I'll land first something minimal to deal with the current problem and reland my patch. I'll send a subsequent patch to fix WeakFixedArray.
Message was sent while issue was closed.
On 2016/12/15 21:30:33, Mircea Trofin wrote: > On 2016/12/15 19:56:03, Yang wrote: > > On 2016/12/15 19:51:09, Mircea Trofin wrote: > > > On 2016/12/15 19:49:15, Yang wrote: > > > > On 2016/12/15 19:35:49, Mircea Trofin wrote: > > > > > A revert of this CL (patchset #1 id:1) has been created in > > > > > https://codereview.chromium.org/2582623002/ by > > mailto:mtrofin@chromium.org. > > > > > > > > > > The reason for reverting is: > > > > > > > > > > > > > > > https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds.... > > > > > > > > Oh yeah I think you need to check upfront whether it's a weak fixed array > at > > > > all. Iirc it's not if there are no shared function infos. > > > > > > Would it be more usable if the check happened in the iterator, or, even > better > > > (imho) if the iterator were appropriately typed? Currently, the constructor > > > accepts an Object* > > > > Yeah that probably helps. But all of this changed here anyways: > > https://codereview.chromium.org/2577063002/diff/20001/src/objects.h > > It's more involved. > > It seems that there's no way to tell if an object is FixedArray or > WeakFixedArray. Then, the semantics of "get" and "set" in a WeakFixedArray are > different from the normal FixedArray, because we use the first index to store > the last accessed index. > > The situation seems fragile. I'll land first something minimal to deal with the > current problem and reland my patch. I'll send a subsequent patch to fix > WeakFixedArray. That is true. But usually you can tell from context whether we are using a WeakFixedArray. There are no cases where we might use either.
Message was sent while issue was closed.
Description was changed from ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 Review-Url: https://codereview.chromium.org/2582583002 Cr-Commit-Position: refs/heads/master@{#41738} Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec... ========== to ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 Review-Url: https://codereview.chromium.org/2582583002 Cr-Commit-Position: refs/heads/master@{#41738} Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec... ==========
The CQ bit was checked by mtrofin@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.
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2582583002/#ps20001 (title: "using new iterator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481868684020850,
"parent_rev": "7333663dc99f125734b00417be1bb1a7ab11988a", "commit_rev":
"93e53da4c846327760c412227a6a5983c9ca86fb"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 Review-Url: https://codereview.chromium.org/2582583002 Cr-Commit-Position: refs/heads/master@{#41738} Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec... ========== to ========== [wasm] simpler detection if we compiled asm-wasm BUG=643595 Review-Url: https://codereview.chromium.org/2582583002 Cr-Original-Commit-Position: refs/heads/master@{#41738} Committed: https://chromium.googlesource.com/v8/v8/+/cb433bed0b940a7073b5215e6684b940eec... Review-Url: https://codereview.chromium.org/2582583002 Cr-Commit-Position: refs/heads/master@{#41743} Committed: https://chromium.googlesource.com/v8/v8/+/93e53da4c846327760c412227a6a5983c9c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/93e53da4c846327760c412227a6a5983c9c... |
