Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2313193002:
[Tracing] Fix runtime call stats tracing for GC. (Closed)
Description[Tracing] Fix runtime call stats tracing for GC.
This patch adds runtime call stats tracing for GC correctly, makes
--runtime-call-stats and tracing mutually exclusive with tracing taking
precedence if both modes are on, and uses only one runtime call stats in
counter.
BUG=v8:5089
Committed: https://crrev.com/252b84b0ed28efc597fb18c10dbea7a67fcff075
Cr-Commit-Position: refs/heads/master@{#39295}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update & rebase #
Total comments: 8
Patch Set 3 : Remove duplicate runtime call stats in counters #
Total comments: 1
Messages
Total messages: 39 (26 generated)
The CQ bit was checked by lpy@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...
lpy@chromium.org changed reviewers: + cbruni@chromium.org, fmeawad@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nit https://codereview.chromium.org/2313193002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2313193002/diff/1/src/counters.h#newcode1264 src/counters.h:1264: RuntimeCallStats::CounterId counter_id); nit: could you declare both constructors in the same file. secondary nit: could you use a delegating constructor for the HeapObject version, this would save some code duplication.
The CQ bit was checked by lpy@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: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12318) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/12254) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8414)
lpy@chromium.org changed reviewers: + jochen@chromium.org, ulan@chromium.org
Thanks. +jochen@ and ulan@ for heap, could you please take a look at it? https://codereview.chromium.org/2313193002/diff/1/src/counters.h File src/counters.h (right): https://codereview.chromium.org/2313193002/diff/1/src/counters.h#newcode1264 src/counters.h:1264: RuntimeCallStats::CounterId counter_id); On 2016/09/07 02:00:29, Camillo Bruni wrote: > nit: could you declare both constructors in the same file. > secondary nit: could you use a delegating constructor for the HeapObject > version, this would save some code duplication. Done.
The CQ bit was checked by lpy@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with some questions. https://codereview.chromium.org/2313193002/diff/40001/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2313193002/diff/40001/src/counters-inl.h#newc... src/counters-inl.h:18: RuntimeCallStats::Enter(isolate_->counters()->tracing_runtime_call_stats(), Do we still need the runtime_call_stats table and the tracing_runtime_call_stats one? https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc File src/heap/gc-tracer.cc (right): https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:30: &timer_, &RuntimeCallStats::GC); This code is repeated 2 times, should we move it to counters?
https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc File src/heap/gc-tracer.cc (right): https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:29: tracer_->heap_->isolate()->counters()->tracing_runtime_call_stats(), Naive question: why can't we use the same counter for tracing runtime calls and ordinary runtime calls? They seem to be mutually exclusive.
jochen@chromium.org changed reviewers: - jochen@chromium.org
deferring to ulan
https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc File src/heap/gc-tracer.cc (right): https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:29: tracer_->heap_->isolate()->counters()->tracing_runtime_call_stats(), On 2016/09/08 at 12:03:37, ulan wrote: > Naive question: why can't we use the same counter for tracing runtime calls and ordinary runtime calls? > > They seem to be mutually exclusive. You're right. Previously they weren't mutually exclusive which would have justified the separate counter lists.
The CQ bit was checked by lpy@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
==========
[Tracing] Fix runtime call stats tracing for GC.
This patch adds runtime call stats tracing for GC correctly, as well as makes
--runtime-call-stats and tracing mutually exclusive with tracing taking
precedence if both modes are on.
BUG=v8:5089
==========
to
==========
[Tracing] Fix runtime call stats tracing for GC.
This patch adds runtime call stats tracing for GC correctly, makes
--runtime-call-stats and tracing mutually exclusive with tracing taking
precedence if both modes are on, and uses only one runtime call stats in
counter.
BUG=v8:5089
==========
Description was changed from
==========
[Tracing] Fix runtime call stats tracing for GC.
This patch adds runtime call stats tracing for GC correctly, makes
--runtime-call-stats and tracing mutually exclusive with tracing taking
precedence if both modes are on, and uses only one runtime call stats in
counter.
BUG=v8:5089
==========
to
==========
[Tracing] Fix runtime call stats tracing for GC.
This patch adds runtime call stats tracing for GC correctly, makes
--runtime-call-stats and tracing mutually exclusive with tracing taking
precedence if both modes are on, and uses only one runtime call stats in
counter.
BUG=v8:5089
==========
Thanks for the feedback, I updated the CL. PTAL. https://codereview.chromium.org/2313193002/diff/40001/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2313193002/diff/40001/src/counters-inl.h#newc... src/counters-inl.h:18: RuntimeCallStats::Enter(isolate_->counters()->tracing_runtime_call_stats(), On 2016/09/07 17:31:28, fmeawad wrote: > Do we still need the runtime_call_stats table and the tracing_runtime_call_stats > one? Done. https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc File src/heap/gc-tracer.cc (right): https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:29: tracer_->heap_->isolate()->counters()->tracing_runtime_call_stats(), On 2016/09/08 12:03:37, ulan wrote: > Naive question: why can't we use the same counter for tracing runtime calls and > ordinary runtime calls? > > They seem to be mutually exclusive. right, this is also what fmeawad@ is suggesting. Done. https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:29: tracer_->heap_->isolate()->counters()->tracing_runtime_call_stats(), On 2016/09/08 15:47:52, Camillo Bruni wrote: > On 2016/09/08 at 12:03:37, ulan wrote: > > Naive question: why can't we use the same counter for tracing runtime calls > and ordinary runtime calls? > > > > They seem to be mutually exclusive. > > You're right. Previously they weren't mutually exclusive which would have > justified the separate counter lists. Done. https://codereview.chromium.org/2313193002/diff/40001/src/heap/gc-tracer.cc#n... src/heap/gc-tracer.cc:30: &timer_, &RuntimeCallStats::GC); On 2016/09/07 17:31:28, fmeawad wrote: > This code is repeated 2 times, should we move it to counters? I don't think we should make another enter and leave in counter.
Thank you for removing the second counter! LGTM.
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 lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org, fmeawad@chromium.org Link to the patchset: https://codereview.chromium.org/2313193002/#ps60001 (title: "Remove duplicate runtime call stats in counters")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Fix runtime call stats tracing for GC. This patch adds runtime call stats tracing for GC correctly, makes --runtime-call-stats and tracing mutually exclusive with tracing taking precedence if both modes are on, and uses only one runtime call stats in counter. BUG=v8:5089 ========== to ========== [Tracing] Fix runtime call stats tracing for GC. This patch adds runtime call stats tracing for GC correctly, makes --runtime-call-stats and tracing mutually exclusive with tracing taking precedence if both modes are on, and uses only one runtime call stats in counter. BUG=v8:5089 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Fix runtime call stats tracing for GC. This patch adds runtime call stats tracing for GC correctly, makes --runtime-call-stats and tracing mutually exclusive with tracing taking precedence if both modes are on, and uses only one runtime call stats in counter. BUG=v8:5089 ========== to ========== [Tracing] Fix runtime call stats tracing for GC. This patch adds runtime call stats tracing for GC correctly, makes --runtime-call-stats and tracing mutually exclusive with tracing taking precedence if both modes are on, and uses only one runtime call stats in counter. BUG=v8:5089 Committed: https://crrev.com/252b84b0ed28efc597fb18c10dbea7a67fcff075 Cr-Commit-Position: refs/heads/master@{#39295} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/252b84b0ed28efc597fb18c10dbea7a67fcff075 Cr-Commit-Position: refs/heads/master@{#39295}
Message was sent while issue was closed.
https://codereview.chromium.org/2313193002/diff/60001/src/counters-inl.h File src/counters-inl.h (right): https://codereview.chromium.org/2313193002/diff/60001/src/counters-inl.h#newc... src/counters-inl.h:27: RuntimeCallTimerScope(heap_object->GetIsolate(), counter_id); You'll ended up checking the flags twice this way ,no? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
