Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807013002:
[wasm] Misc fixes for async compilation (Closed)
|
Created:
3 years, 8 months ago by Mircea Trofin Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionFixed accounting issues due to code table containing imports as well as wasm funcs.
Ensuring we move forward all the deferred handles, in all cases.
BUG=
Review-Url: https://codereview.chromium.org/2807013002
Cr-Commit-Position: refs/heads/master@{#44525}
Committed: https://chromium.googlesource.com/v8/v8/+/85b1f108c577fe5ef78890da26fd71db5eddc8a7
Patch Set 1 #Patch Set 2 : [wasm] Fix async compile for empty modules, or modules with tables. #
Total comments: 2
Patch Set 3 : rename #
Messages
Total messages: 25 (15 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...
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] test for 709684 BUG= ========== to ========== Fixed accounting issues due to code table containing imports as well as wasm funcs. Ensuring we move forward all the deferred handles, in all cases. BUG=709684 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtrofin@chromium.org changed reviewers: + ahaas@chromium.org, bradnelson@chromium.org, kschimpf@chromium.org
Ptal. I think this fixes the quoted bug, will validate shortly on chrome side
lgtm. Thanks for looking into this!
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
On 2017/04/09 15:19:28, kschimpf wrote: > lgtm. > > Thanks for looking into this! Attempted to verify back in Chrome, while this CL fixes something (see what fails in Patch 1), it doesn't fix the initial bug.
On 2017/04/09 18:20:46, Mircea Trofin wrote: > On 2017/04/09 15:19:28, kschimpf wrote: > > lgtm. > > > > Thanks for looking into this! > > Attempted to verify back in Chrome, while this CL fixes something (see what > fails in Patch 1), it doesn't fix the initial bug. Figured what the problem in 709684 is - we need to initialize wasm counters on a thread that's joinable. I suppose a good place for that is at isolate setup. Working on the fix.
Thanks for this fix! LGTM https://codereview.chromium.org/2807013002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2807013002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2834: MoveHandlesToDeferred(); The usual name that is used elsewhere in V8 is {ReopenHandles},
Description was changed from ========== Fixed accounting issues due to code table containing imports as well as wasm funcs. Ensuring we move forward all the deferred handles, in all cases. BUG=709684 ========== to ========== Fixed accounting issues due to code table containing imports as well as wasm funcs. Ensuring we move forward all the deferred handles, in all cases. BUG= ==========
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kschimpf@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2807013002/#ps40001 (title: "rename")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2807013002/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2807013002/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2834: MoveHandlesToDeferred(); On 2017/04/10 07:01:38, ahaas wrote: > The usual name that is used elsewhere in V8 is {ReopenHandles}, Thanks - I assume you mean ReopenHandles as a suffix, like in ParseInfo's ReopenHandlesInNewScope. Done!
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491831303398610,
"parent_rev": "72d5f3848ec1ad0ae07b9bf7b049b1a1f2e7f81d", "commit_rev":
"85b1f108c577fe5ef78890da26fd71db5eddc8a7"}
Message was sent while issue was closed.
Description was changed from ========== Fixed accounting issues due to code table containing imports as well as wasm funcs. Ensuring we move forward all the deferred handles, in all cases. BUG= ========== to ========== Fixed accounting issues due to code table containing imports as well as wasm funcs. Ensuring we move forward all the deferred handles, in all cases. BUG= Review-Url: https://codereview.chromium.org/2807013002 Cr-Commit-Position: refs/heads/master@{#44525} Committed: https://chromium.googlesource.com/v8/v8/+/85b1f108c577fe5ef78890da26fd71db5ed... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/85b1f108c577fe5ef78890da26fd71db5ed... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
