Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 9 months ago by Stefano Sanfilippo Modified:
4 years, 9 months ago Reviewers:
rmcilroy, Yang, Michael Starzinger CC:
v8-reviews_googlegroups.com, oth, Yang Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Log code-creation events for bytecode handlers.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/8447072d605f08f7b2c3f3415891648782d55c35
Cr-Commit-Position: refs/heads/master@{#34631}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Review. #
Total comments: 2
Patch Set 3 : Bytecode handlers should not appear in code serializer. #Patch Set 4 : Rebasing before committing. #
Created: 4 years, 9 months ago
Messages
Total messages: 38 (18 generated)
ssanfilippo@chromium.org changed reviewers: + mstarzinger@chromium.org, oth@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org
https://codereview.chromium.org/1772403002/diff/1/src/log.h File src/log.h (right): https://codereview.chromium.org/1772403002/diff/1/src/log.h#newcode85 src/log.h:85: #define LOG_EVENTS_AND_TAGS_LIST(V) \ git cl format did this, sorry.
Description was changed from ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: - mstarzinger@chromium.org, oth@chromium.org
ssanfilippo@chromium.org changed reviewers: + mstarzinger@chromium.org
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/1
Looks good from my point of view with two comments. I'll let Yang and / or Michi approve. https://codereview.chromium.org/1772403002/diff/1/src/interpreter/interpreter.h File src/interpreter/interpreter.h (right): https://codereview.chromium.org/1772403002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.h:23: class Logger; unnecessary https://codereview.chromium.org/1772403002/diff/1/src/log.cc File src/log.cc (right): https://codereview.chromium.org/1772403002/diff/1/src/log.cc#newcode1605 src/log.cc:1605: void Logger::LogBytecodeHandlers() { You should probably guard on FLAG_ignition so this only does the logging when --ignition is passed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/...)
https://codereview.chromium.org/1772403002/diff/1/src/interpreter/interpreter.h File src/interpreter/interpreter.h (right): https://codereview.chromium.org/1772403002/diff/1/src/interpreter/interpreter... src/interpreter/interpreter.h:23: class Logger; On 2016/03/08 16:21:57, rmcilroy wrote: > unnecessary Done. https://codereview.chromium.org/1772403002/diff/1/src/log.cc File src/log.cc (right): https://codereview.chromium.org/1772403002/diff/1/src/log.cc#newcode1605 src/log.cc:1605: void Logger::LogBytecodeHandlers() { On 2016/03/08 16:21:57, rmcilroy wrote: > You should probably guard on FLAG_ignition so this only does the logging when > --ignition is passed. Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm once comment is addressed. https://codereview.chromium.org/1772403002/diff/20001/src/snapshot/code-seria... File src/snapshot/code-serializer.cc (right): https://codereview.chromium.org/1772403002/diff/20001/src/snapshot/code-seria... src/snapshot/code-serializer.cc:74: case Code::BYTECODE_HANDLER: Please move this to the CHECK(false) cases above. This cannot and must not happen. The code cache must not contain any references directly to the bytecode handlers.
https://codereview.chromium.org/1772403002/diff/20001/src/snapshot/code-seria... File src/snapshot/code-serializer.cc (right): https://codereview.chromium.org/1772403002/diff/20001/src/snapshot/code-seria... src/snapshot/code-serializer.cc:74: case Code::BYTECODE_HANDLER: On 2016/03/09 13:02:18, Yang wrote: > Please move this to the CHECK(false) cases above. This cannot and must not > happen. The code cache must not contain any references directly to the bytecode > handlers. Done.
The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/40001
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 ssanfilippo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/60001
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 ssanfilippo@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/1772403002/#ps60001 (title: "Rebasing before committing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12114)
LGTM, thanks.
The CQ bit was checked by ssanfilippo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772403002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772403002/60001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Log code-creation events for bytecode handlers. BUG=v8:4280 LOG=N Committed: https://crrev.com/8447072d605f08f7b2c3f3415891648782d55c35 Cr-Commit-Position: refs/heads/master@{#34631} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8447072d605f08f7b2c3f3415891648782d55c35 Cr-Commit-Position: refs/heads/master@{#34631} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
