Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1634653002:
[wasm] register WASM code creation event for profilers (Closed)
Description[wasm] register WASM code creation event for profilers
Also remove duplicate code Disassemble, which is already done in TF pipeline.
BUG=
Committed: https://crrev.com/5c7134a98e3ec2cf5dfb66509e1a709c54d5fe0d
Cr-Commit-Position: refs/heads/master@{#33610}
Patch Set 1 #Patch Set 2 : fixed crash #
Total comments: 8
Patch Set 3 : addressed comments #Patch Set 4 : rebased #Patch Set 5 : GetName only checks bounds when there is a memory section since some wasm test cases does not have … #Messages
Total messages: 21 (8 generated)
Description was changed from ========== [wasm] register WASM code creation event for profiler BUG= ========== to ========== [wasm] register WASM code creation event for profilers Also remove duplicate code Disassemble, which is already done in TF pipeline. BUG= ==========
weiliang.lin@intel.com changed reviewers: + bradnelson@chromium.org, danno@chromium.org, titzer@chromium.org
PTAL
https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2076: if (info.isolate()->logger()->is_logging_code_events() || Could you please turn this combined test into a separate predicate since it shows up several times in this file?
https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:1937: Handle<SharedFunctionInfo> shared = You should probably use the shared function info from the JSFunction created above. https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2024: ScopedVector<char> buffer(128); This will always build this string, even when it is not used. https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2063: SNPrintF(buffer, "Compiling %s failed:", buffer.start()); This probably corrupts the buffer, since it uses the buffer as the source and target for formatting.
Thanks for review. Please take a look at the patch set 3. Thanks -Weiliang https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:1937: Handle<SharedFunctionInfo> shared = On 2016/01/26 15:42:31, titzer wrote: > You should probably use the shared function info from the JSFunction created > above. the shared function info here is the js-to-wasm trampoline. The previous one is the wasm function. Here we record the trampoline code creation. https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2024: ScopedVector<char> buffer(128); On 2016/01/26 15:42:31, titzer wrote: > This will always build this string, even when it is not used. Done. https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2063: SNPrintF(buffer, "Compiling %s failed:", buffer.start()); On 2016/01/26 15:42:31, titzer wrote: > This probably corrupts the buffer, since it uses the buffer as the source and > target for formatting. Done. https://codereview.chromium.org/1634653002/diff/20001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:2076: if (info.isolate()->logger()->is_logging_code_events() || On 2016/01/26 08:40:06, danno wrote: > Could you please turn this combined test into a separate predicate since it > shows up several times in this file? Done.
lgtm
The CQ bit was checked by weiliang.lin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1634653002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1634653002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
Patchset #4 (id:60001) has been deleted
@titzer, PTAL. Fixed BoundsCheck failure when getting function name with function name offset. Memory section may miss in the test cases, so Bounds Check is only done only when the offset is not zero. Thanks -Weiliang
On 2016/01/29 05:29:07, Weiliang wrote: > @titzer, PTAL. Fixed BoundsCheck failure when getting function name with > function name offset. Memory section may miss in the test cases, so Bounds Check > is only done only when the offset is not zero. > > > Thanks > -Weiliang lgtm
The CQ bit was checked by weiliang.lin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1634653002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1634653002/100001
Message was sent while issue was closed.
Description was changed from ========== [wasm] register WASM code creation event for profilers Also remove duplicate code Disassemble, which is already done in TF pipeline. BUG= ========== to ========== [wasm] register WASM code creation event for profilers Also remove duplicate code Disassemble, which is already done in TF pipeline. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] register WASM code creation event for profilers Also remove duplicate code Disassemble, which is already done in TF pipeline. BUG= ========== to ========== [wasm] register WASM code creation event for profilers Also remove duplicate code Disassemble, which is already done in TF pipeline. BUG= Committed: https://crrev.com/5c7134a98e3ec2cf5dfb66509e1a709c54d5fe0d Cr-Commit-Position: refs/heads/master@{#33610} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5c7134a98e3ec2cf5dfb66509e1a709c54d5fe0d Cr-Commit-Position: refs/heads/master@{#33610} |
