Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503183002:
[Tracing] Implement IC statistics in tracing. (Closed)
Description[Tracing] Implement IC statistics in tracing.
This patch introduces:
1. ICStats class to store ic statistics items produced by V8,
2. A disabled by default tracing category v8.ic_stats,
3. An trace event V8.ICStats that contains ic statistics items in args,
We store ic statistics items in an array until the array is full to reduce
the number of trace events.
TBR=jkummerow@chromium.org,ishell@chromium.org
Committed: https://crrev.com/0a3c8fc3ef7cf2c408244d9b67349c1aac1265bb
Cr-Commit-Position: refs/heads/master@{#41559}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Cache script name #Patch Set 4 : update #Patch Set 5 : add cache for function name #
Total comments: 31
Patch Set 6 : Address cbruni's comments #
Total comments: 2
Patch Set 7 : Remove unnecessary flag check #Patch Set 8 : update #Patch Set 9 : update #
Total comments: 6
Patch Set 10 : Remove unnecessary cast #
Messages
Total messages: 74 (62 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17817)
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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18147) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) 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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16554)
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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18155) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18268)
Patchset #3 (id:40001) has been deleted
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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/18259)
The CQ bit was checked by lpy@chromium.org
The CQ bit was unchecked by lpy@chromium.org
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29801)
lpy@chromium.org changed reviewers: + cbruni@chromium.org, fmeawad@chromium.org, ishell@chromium.org, jkummerow@chromium.org
PTAL. This will be the first version of IC stats in tracing. We plan to iterate on it to use pc to infer function name, etc, so that we don't blow the size of the patch.
first round of comments :) https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc#newc... src/code-stubs.cc:2296: if (FLAG_ic_stats & no need to check the flag again. https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc#newc... src/code-stubs.cc:2306: } else if (FLAG_ic_stats) { same here. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc File src/ic/ic-stats.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:64: String* script_name = String::cast(script->name()); String::cast(script_name_raw); https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:84: std::string function_name = function->IsOptimized() ? "*" : "~"; that might be another boolean that could be put no the ICInfo struct WDYT? https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:87: return function_name_map_[function]; return function_name; https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:128: value->SetString("map", ss.str()); Can't you just use SetInteger and reinterpet_cast the `void* map` to an int? https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h File src/ic/ic-stats.h (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:33: int offset; rename to script_offset https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:36: unsigned constructor; bool is_constructor https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:41: unsigned dict; this should be bool I guess (also maybe rename it to something more clear like is_dictionary_map) https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:43: unsigned own; please rename to: number_of_own_descriptors (or a short form of it). https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:66: base::Atomic32 being_used_; How about "enabled"? https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:69: std::unordered_map<JSFunction*, std::string> function_name_map_; Did you try running this on a big website and see if the memory consumption isn't killing us? http://edition.cnn.com/ is reasonably complex and good old http://facebook.com/shakira? https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc File src/ic/ic.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode108 src/ic/ic.cc:108: if (FLAG_ic_stats & no need to recheck the flag https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode114 src/ic/ic.cc:114: } else if (FLAG_ic_stats) { same here. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode158 src/ic/ic.cc:158: ic_info.state = "("; I'm not too familiar with the C++ standard library, but maybe it would help to do std::string::reserve upfront.
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 #6 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
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...
Thanks for the comments, ptal. The change in traced-value.[h|cc] will be landed in separate CL. https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc#newc... src/code-stubs.cc:2296: if (FLAG_ic_stats & On 2016/12/02 12:12:55, Camillo Bruni wrote: > no need to check the flag again. but we want to check if the flag is enabled by tracing or not, and if the flag is set and not enabled by tracing, then it's enabled by command line. https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc#newc... src/code-stubs.cc:2306: } else if (FLAG_ic_stats) { On 2016/12/02 12:12:55, Camillo Bruni wrote: > same here. see explanation above. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc File src/ic/ic-stats.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:64: String* script_name = String::cast(script->name()); On 2016/12/02 12:12:55, Camillo Bruni wrote: > String::cast(script_name_raw); Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:84: std::string function_name = function->IsOptimized() ? "*" : "~"; On 2016/12/02 12:12:55, Camillo Bruni wrote: > that might be another boolean that could be put no the ICInfo struct WDYT? Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:87: return function_name_map_[function]; On 2016/12/02 12:12:55, Camillo Bruni wrote: > return function_name; Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.cc#new... src/ic/ic-stats.cc:128: value->SetString("map", ss.str()); On 2016/12/02 12:12:55, Camillo Bruni wrote: > Can't you just use SetInteger and reinterpet_cast the `void* map` to an int? Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h File src/ic/ic-stats.h (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:33: int offset; On 2016/12/02 12:12:55, Camillo Bruni wrote: > rename to script_offset Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:36: unsigned constructor; On 2016/12/02 12:12:55, Camillo Bruni wrote: > bool is_constructor Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:41: unsigned dict; On 2016/12/02 12:12:55, Camillo Bruni wrote: > this should be bool I guess (also maybe rename it to something more clear like > is_dictionary_map) Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:43: unsigned own; On 2016/12/02 12:12:55, Camillo Bruni wrote: > please rename to: number_of_own_descriptors (or a short form of it). Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:66: base::Atomic32 being_used_; On 2016/12/02 12:12:55, Camillo Bruni wrote: > How about "enabled"? Done. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic-stats.h#newc... src/ic/ic-stats.h:69: std::unordered_map<JSFunction*, std::string> function_name_map_; On 2016/12/02 12:12:55, Camillo Bruni wrote: > Did you try running this on a big website and see if the memory consumption > isn't killing us? > http://edition.cnn.com/ is reasonably complex and good old > http://facebook.com/shakira Thanks for the advice, will try it with Chrome TOT and let you know. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc File src/ic/ic.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode108 src/ic/ic.cc:108: if (FLAG_ic_stats & On 2016/12/02 12:12:55, Camillo Bruni wrote: > no need to recheck the flag same here, we want to check whether the flag is enabled by tracing, and if the flag is set but not enabled by tracing, then it's enabled by command line. https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode114 src/ic/ic.cc:114: } else if (FLAG_ic_stats) { On 2016/12/02 12:12:55, Camillo Bruni wrote: > same here. see explanation above https://codereview.chromium.org/2503183002/diff/100001/src/ic/ic.cc#newcode158 src/ic/ic.cc:158: ic_info.state = "("; On 2016/12/02 12:12:55, Camillo Bruni wrote: > I'm not too familiar with the C++ standard library, but maybe it would help to > do std::string::reserve upfront. Done.
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/2503183002/diff/100001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2503183002/diff/100001/src/code-stubs.cc#newc... src/code-stubs.cc:2306: } else if (FLAG_ic_stats) { On 2016/12/05 at 17:49:10, lpy wrote: > On 2016/12/02 12:12:55, Camillo Bruni wrote: > > same here. > > see explanation above. Argh, sorry I misread the single & above, but nevertheless, you can drop this second check then as this will be certainly true. https://codereview.chromium.org/2503183002/diff/140001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2503183002/diff/140001/src/code-stubs.cc#newc... src/code-stubs.cc:2306: } else if (FLAG_ic_stats) { nit: drop this check as you the early return above will ensure FLAG_ic_stats != 0.
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: This issue passed the CQ dry run.
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: + alph@chromium.org
alph@ pointed out that we shouldn't output number that would be larger than 2^52 to JSON. Thus in order to make sure we can represent it as an address, I will use the old way by using stringstream. https://codereview.chromium.org/2503183002/diff/140001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/2503183002/diff/140001/src/code-stubs.cc#newc... src/code-stubs.cc:2306: } else if (FLAG_ic_stats) { On 2016/12/05 20:13:50, Camillo Bruni wrote: > nit: drop this check as you the early return above will ensure FLAG_ic_stats != > 0. Done.
Patchset #8 (id:180001) has been deleted
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: This issue passed the CQ dry run.
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: This issue passed the CQ dry run.
Some nits. https://codereview.chromium.org/2503183002/diff/220001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2503183002/diff/220001/src/flag-definitions.h... src/flag-definitions.h:809: "inline cache state transitions statistics for tracing only") nit: Is it "for tracing only"? It is for tracing only if set directly https://codereview.chromium.org/2503183002/diff/220001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/2503183002/diff/220001/src/frames.cc#newcode1060 src/frames.cc:1060: void JavaScriptFrame::CollectFunctionAndOffsetForICStats(JSFunction* function, I think the Print functions should be using these 2 methods, is there a technical reason why we should not? is it a different format? If so then we should collect then format. https://codereview.chromium.org/2503183002/diff/220001/src/frames.h File src/frames.h (right): https://codereview.chromium.org/2503183002/diff/220001/src/frames.h#newcode920 src/frames.h:920: static void CollectFunctionAndOffsetForICStats(JSFunction* function, nit: I don't think you should include "ForICStats" in the name. https://codereview.chromium.org/2503183002/diff/220001/src/frames.h#newcode923 src/frames.h:923: static void CollectTopFrameForICStats(Isolate* isolate); nit: ditto
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...
https://codereview.chromium.org/2503183002/diff/220001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2503183002/diff/220001/src/flag-definitions.h... src/flag-definitions.h:809: "inline cache state transitions statistics for tracing only") On 2016/12/06 00:59:12, fmeawad wrote: > nit: Is it "for tracing only"? It is for tracing only if set directly Done. https://codereview.chromium.org/2503183002/diff/220001/src/frames.cc File src/frames.cc (right): https://codereview.chromium.org/2503183002/diff/220001/src/frames.cc#newcode1060 src/frames.cc:1060: void JavaScriptFrame::CollectFunctionAndOffsetForICStats(JSFunction* function, On 2016/12/06 00:59:12, fmeawad wrote: > I think the Print functions should be using these 2 methods, is there a > technical reason why we should not? is it a different format? If so then we > should collect then format. The only heavy logic shared by Print functions and IC stats specific methods is that they both construct script name and function name, however, we use maps to cache script names and function names for IC stats. I agree that we should unify them if possible, but since we are going to move to use pc eventually, I think it's fine for now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
jkummerow@, ishell@, gentle ping for review.
Description was changed from ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. ========== to ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. TBR=jkummerow@chromium.org,ishell@chromium.org ==========
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 Link to the patchset: https://codereview.chromium.org/2503183002/#ps240001 (title: "Remove unnecessary cast")
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": 240001, "attempt_start_ts": 1481129772927120,
"parent_rev": "8ccb36d6818b5017365d7df7fd253bc3bb489de0", "commit_rev":
"96e145df0050a59dc4cb37a61f236997c457f5af"}
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. TBR=jkummerow@chromium.org,ishell@chromium.org ========== to ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. TBR=jkummerow@chromium.org,ishell@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. TBR=jkummerow@chromium.org,ishell@chromium.org ========== to ========== [Tracing] Implement IC statistics in tracing. This patch introduces: 1. ICStats class to store ic statistics items produced by V8, 2. A disabled by default tracing category v8.ic_stats, 3. An trace event V8.ICStats that contains ic statistics items in args, We store ic statistics items in an array until the array is full to reduce the number of trace events. TBR=jkummerow@chromium.org,ishell@chromium.org Committed: https://crrev.com/0a3c8fc3ef7cf2c408244d9b67349c1aac1265bb Cr-Commit-Position: refs/heads/master@{#41559} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0a3c8fc3ef7cf2c408244d9b67349c1aac1265bb Cr-Commit-Position: refs/heads/master@{#41559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
