Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778493002:
Separate module compile counter for asm and wasm. (Closed)
DescriptionSeparate module compile counter for asm and wasm.
Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module.
Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once.
BUG=chromium:704922
R=bradnelson@chromium.org,bbudge@chromium.org
Review-Url: https://codereview.chromium.org/2778493002
Cr-Commit-Position: refs/heads/master@{#44126}
Committed: https://chromium.googlesource.com/v8/v8/+/ba40bf7a42324d3e58a013e41683ee207f392a0b
Patch Set 1 #
Total comments: 2
Messages
Total messages: 22 (12 generated)
The CQ bit was checked by kschimpf@google.com 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.
aseemgarg@chromium.org changed reviewers: + aseemgarg@chromium.org
lgtm
The CQ bit was checked by kschimpf@google.com
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/37631)
Need an owner to approve.
https://codereview.chromium.org/2778493002/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2778493002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:517: module_->origin == ModuleOrigin::kWasmOrigin Since we'll be doing this check all over the place, it's probably worth pre-computing this boolean in the constructor and keeping it as an instance field. You can add a DCHECK that it's either kWasmOrigin or kAsmJsOrigin to prevent breakage, something like: is_wasm_origin_ = module_->origin == ModuleOrigin::kWasmOrigin; DCHECK_IMPLIES(!is_wasm_origin_, module_origin == ModuleOrigin::kAsmJsOrigin); Then just use the instance field everywhere you need to switch between counters.
Description was changed from ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 R=bradnelson@chromium.org,bbudge@chromium.org ========== to ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 BUG=v8:6152 R=bradnelson@chromium.org,bbudge@chromium.org ==========
Description was changed from ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 BUG=v8:6152 R=bradnelson@chromium.org,bbudge@chromium.org ========== to ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 R=bradnelson@chromium.org,bbudge@chromium.org ==========
Adding field accessor for WasmModule.origin in separate CL. https://codereview.chromium.org/2778493002/diff/1/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2778493002/diff/1/src/wasm/wasm-module.cc#new... src/wasm/wasm-module.cc:517: module_->origin == ModuleOrigin::kWasmOrigin On 2017/03/24 19:19:01, bbudge wrote: > Since we'll be doing this check all over the place, it's probably worth > pre-computing this boolean in the constructor and keeping it as an instance > field. You can add a DCHECK that it's either kWasmOrigin or kAsmJsOrigin to > prevent breakage, something like: > > is_wasm_origin_ = module_->origin == ModuleOrigin::kWasmOrigin; > DCHECK_IMPLIES(!is_wasm_origin_, module_origin == ModuleOrigin::kAsmJsOrigin); > > Then just use the instance field everywhere you need to switch between counters. Part of the problem is that the origin field is NOT set in the constructor. However, we should still use accessors like is_wasm() and is_asm_js() (as well as get_origin() and set_origin()). This would allow us to cache a boolean without having to change any code other than for the class definition. I'll do that.
The CQ bit was checked by bradnelson@chromium.org
lgtm
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": 1490390780091410, "parent_rev":
"74c7dd0badd30c0bc1cc9199e12786c6341f679b", "commit_rev":
"ba40bf7a42324d3e58a013e41683ee207f392a0b"}
Message was sent while issue was closed.
Description was changed from ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 R=bradnelson@chromium.org,bbudge@chromium.org ========== to ========== Separate module compile counter for asm and wasm. Currently, v8 uses the same flag to collect time for compiling wasm modules from js asm and wasm. This separates the v8 counter into two separate counters, and then uses the appropriate counter when compiling a module. Note: This CL doesn't separate the corresponding Chromium counter. Rather that is intenionally being delayed until v8 code has separated all appropriate counters for js asm and wasm. That way, the installation into chrome can be done at once. BUG=chromium:704922 R=bradnelson@chromium.org,bbudge@chromium.org Review-Url: https://codereview.chromium.org/2778493002 Cr-Commit-Position: refs/heads/master@{#44126} Committed: https://chromium.googlesource.com/v8/v8/+/ba40bf7a42324d3e58a013e41683ee207f3... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/v8/v8/+/ba40bf7a42324d3e58a013e41683ee207f3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
