Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1899133004:
[Interpreter] Add Ignition statistics JavaScript extension. (Closed)
|
Created:
4 years, 8 months ago by Stefano Sanfilippo Modified:
4 years, 7 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[Interpreter] Add Ignition statistics JavaScript extension.
This commit introduces IgnitionStatisticsExtension, which provides
methods for accessing Ignition statistics and counters from JavaScript.
The extension is registered when FLAG_ignition and
FLAG_trace_ignition_dispatches are both enabled.
For the moment, the only exposed function is
getIgnitionDispatchCounters(), which allows to retrieve Ignition
dispatch counters as a JavaScript object.
BUG=v8:4899
LOG=N
Committed: https://crrev.com/905becd13b8696e126255decf130fdb9e1d9aa30
Cr-Commit-Position: refs/heads/master@{#35816}
Patch Set 1 : #
Total comments: 17
Patch Set 2 : Review. #Patch Set 3 : Reintroduce CHECKs. #Patch Set 4 : Use JSON::Stringify. #Patch Set 5 : Renaming for consistency. #Patch Set 6 : Change return type of bytecode_dispatch_counters_table() #
Total comments: 4
Patch Set 7 : Rebase, make Interpreter::GetDispatchCounter private. #Patch Set 8 : Rebase (sync src/v8.gyp) #Patch Set 9 #Patch Set 10 #Patch Set 11 : Added mjsunit test. #Patch Set 12 : Check that the outer object has at least one property. #Patch Set 13 : Rebase. #
Messages
Total messages: 93 (50 generated)
Description was changed from ========== [Interpreter] Add extension to dump Ignition dispatch counters in JS. TODO BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add extension to dump Ignition dispatch counters in JS. TODO BUG=v8:4899 LOG=N ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== [Interpreter] Add extension to dump Ignition dispatch counters in JS. TODO BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition is enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the ignition dispatches counters when FLAG_trace_ignition_dispatches is enabled. BUG=v8:4899 LOG=N ==========
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition is enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the ignition dispatches counters when FLAG_trace_ignition_dispatches is enabled. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition is enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters when FLAG_trace_ignition_dispatches is enabled. BUG=v8:4899 LOG=N ==========
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/1899133004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/100001
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition is enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters when FLAG_trace_ignition_dispatches is enabled. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters. BUG=v8:4899 LOG=N ==========
Patchset #1 (id:100001) has been deleted
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/1899133004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/120001
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. This extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters. BUG=v8:4899 LOG=N ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
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/1899133004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/160001
ssanfilippo@chromium.org changed reviewers: + oth@chromium.org, rmcilroy@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Very nice, lgtm, with a couple of comments. It might be an idea to run it by someone with more experience in this area too, jochen or michi, look like good candidates. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... File src/extensions/ignition-statistics-extension.cc (right): https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:31: using interpreter::Bytecode; Is this needed? https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:33: static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1; This could be brittle as the code is reading directly from the dispatch table array so there is no bounds checking. If the code could decouple the dispatch counter table layout that would be better. For instance, the interpreter could expose a get_dispatch_count(from, to) method.
Nice, looks good. Yeah, let's get Jochen or someone to have a look at it once we are happy with it. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... File src/extensions/ignition-statistics-extension.cc (right): https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:26: void IgnitionStatisticsExtension::DumpIgnitionDispatchCounters( Call this the same as the javascript function (or vice versa). https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:33: static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1; On 2016/04/21 14:03:02, oth wrote: > This could be brittle as the code is reading directly from the dispatch table > array so there is no bounds checking. > > If the code could decouple the dispatch counter table layout that would be > better. For instance, the interpreter could expose a get_dispatch_count(from, > to) method. +1 https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:58: v8::Number::New(isolate, static_cast<uint32_t>(counter)); You are casting to 32bits here - did casting to doubles to preserve 64bit values not work? https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:59: CHECK(counters_row->Set(context, to_name_object, counter_object) No need for the CHECK here I don't think. IsJust will crash itself if the return is Nothing. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:71: CHECK(counters_map->Set(context, from_name_object, counters_row).IsJust()); ditto
https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... File src/extensions/ignition-statistics-extension.cc (right): https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:26: void IgnitionStatisticsExtension::DumpIgnitionDispatchCounters( On 2016/04/21 14:17:29, rmcilroy wrote: > Call this the same as the javascript function (or vice versa). Oops, forgot to rename the method as well. Done. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:31: using interpreter::Bytecode; On 2016/04/21 14:03:02, oth wrote: > Is this needed? No, this is not needed, but using interpreter::Bytecode causes a few statements to be laid out on two lines, which makes the code definitely less readable. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:33: static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1; On 2016/04/21 14:03:02, oth wrote: > This could be brittle as the code is reading directly from the dispatch table > array so there is no bounds checking. > > If the code could decouple the dispatch counter table layout that would be > better. For instance, the interpreter could expose a get_dispatch_count(from, > to) method. This is a very good point we might want to discuss further. The same pattern is used elsewhere in the codebase, so I might save it for a future CL. WDYT? https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:58: v8::Number::New(isolate, static_cast<uint32_t>(counter)); On 2016/04/21 14:17:29, rmcilroy wrote: > You are casting to 32bits here - did casting to doubles to preserve 64bit values > not work? Fixed. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:59: CHECK(counters_row->Set(context, to_name_object, counter_object) On 2016/04/21 14:17:29, rmcilroy wrote: > No need for the CHECK here I don't think. IsJust will crash itself if the return > is Nothing. Done. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:71: CHECK(counters_map->Set(context, from_name_object, counters_row).IsJust()); On 2016/04/21 14:17:29, rmcilroy wrote: > ditto Done.
On 2016/04/21 14:17:29, rmcilroy wrote: > https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... > src/extensions/ignition-statistics-extension.cc:59: > CHECK(counters_row->Set(context, to_name_object, counter_object) > No need for the CHECK here I don't think. IsJust will crash itself if the return > is Nothing. Digging in the codebase, it does seem that IsJust() just returns an internal boolean flag, with no check or possibility to crash (and it makes sense, given the name).
https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... File src/extensions/ignition-statistics-extension.cc (right): https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:31: using interpreter::Bytecode; On 2016/04/21 14:22:42, Stefano Sanfilippo wrote: > On 2016/04/21 14:03:02, oth wrote: > > Is this needed? > > No, this is not needed, but using interpreter::Bytecode causes a few statements > to be laid out on two lines, which makes the code definitely less readable. +1 to removing this. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:33: static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1; On 2016/04/21 14:22:42, Stefano Sanfilippo wrote: > On 2016/04/21 14:03:02, oth wrote: > > This could be brittle as the code is reading directly from the dispatch table > > array so there is no bounds checking. > > > > If the code could decouple the dispatch counter table layout that would be > > better. For instance, the interpreter could expose a get_dispatch_count(from, > > to) method. > > This is a very good point we might want to discuss further. The same pattern is > used elsewhere in the codebase, so I might save it for a future CL. WDYT? No I think we should do it here. The only other place which uses bytecode_dispatch_count_table() is in interpreter.cc so we can refactor that usage and remove bytecode_dispatch_count_table() form the interpreter API entirely. Actually, come to think of it, could you move this code into interpreter.cc and then have the d8 version call it to get a JavaScript object then call a function to output it as JSON, instead of manually building the JSON itself?
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #4 (id:260001) has been deleted
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/1899133004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/280001
Patchset #4 (id:280001) has been deleted
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/1899133004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:340001) has been deleted
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/1899133004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... File src/extensions/ignition-statistics-extension.cc (right): https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:31: using interpreter::Bytecode; On 2016/04/21 14:33:30, rmcilroy wrote: > On 2016/04/21 14:22:42, Stefano Sanfilippo wrote: > > On 2016/04/21 14:03:02, oth wrote: > > > Is this needed? > > > > No, this is not needed, but using interpreter::Bytecode causes a few > statements > > to be laid out on two lines, which makes the code definitely less readable. > > +1 to removing this. Done contextually with extracting this piece of code to an Interpreter method. https://codereview.chromium.org/1899133004/diff/160001/src/extensions/ignitio... src/extensions/ignition-statistics-extension.cc:33: static const int kBytecodeCount = static_cast<int>(Bytecode::kLast) + 1; On 2016/04/21 14:33:30, rmcilroy wrote: > On 2016/04/21 14:22:42, Stefano Sanfilippo wrote: > > On 2016/04/21 14:03:02, oth wrote: > > > This could be brittle as the code is reading directly from the dispatch > table > > > array so there is no bounds checking. > > > > > > If the code could decouple the dispatch counter table layout that would be > > > better. For instance, the interpreter could expose a > get_dispatch_count(from, > > > to) method. > > > > This is a very good point we might want to discuss further. The same pattern > is > > used elsewhere in the codebase, so I might save it for a future CL. WDYT? > > No I think we should do it here. The only other place which uses > bytecode_dispatch_count_table() is in interpreter.cc so we can refactor that > usage and remove bytecode_dispatch_count_table() form the interpreter API > entirely. As discussed, we still need a getter for the raw pointer because of the ExternalReference. > Actually, come to think of it, could you move this code into interpreter.cc and > then have the d8 version call it to get a JavaScript object then call a function > to output it as JSON, instead of manually building the JSON itself? This is a brilliant suggestion, done.
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve the Ignition dispatches counters. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters. BUG=v8:4899 LOG=N ==========
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters as a JavaScript object. BUG=v8:4899 LOG=N ==========
ssanfilippo@chromium.org changed reviewers: + machenbach@chromium.org
machenbach please review changes in tools/gyp/v8.gyp
https://codereview.chromium.org/1899133004/diff/380001/tools/gyp/v8.gyp File tools/gyp/v8.gyp (right): https://codereview.chromium.org/1899133004/diff/380001/tools/gyp/v8.gyp#newco... tools/gyp/v8.gyp:776: '../../src/extensions/ignition-statistics-extension.cc', Please add this also to src/v8.gyp so that they stay in sync
LGTM, but as mentioned above, please get someone familiar to the extensions code to take a look before landing. https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... File src/interpreter/interpreter.h (right): https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... src/interpreter/interpreter.h:52: uintptr_t GetDispatchCounter(Bytecode from, Bytecode to) const; This can be private
https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... File src/interpreter/interpreter.h (right): https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... src/interpreter/interpreter.h:52: uintptr_t GetDispatchCounter(Bytecode from, Bytecode to) const; On 2016/04/25 13:05:45, rmcilroy wrote: > This can be private Done.
On 2016/04/25 13:24:07, Stefano Sanfilippo wrote: > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > File src/interpreter/interpreter.h (right): > > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > src/interpreter/interpreter.h:52: uintptr_t GetDispatchCounter(Bytecode from, > Bytecode to) const; > On 2016/04/25 13:05:45, rmcilroy wrote: > > This can be private > > Done. Just as a heads-up, tools/gyp/v8.gyp is now src/v8.gyp.
ssanfilippo@chromium.org changed reviewers: + yangguo@chromium.org
ssanfilippo@chromium.org changed required reviewers: + yangguo@chromium.org
On 2016/04/25 14:09:52, oth wrote: > On 2016/04/25 13:24:07, Stefano Sanfilippo wrote: > > > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > > File src/interpreter/interpreter.h (right): > > > > > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > > src/interpreter/interpreter.h:52: uintptr_t GetDispatchCounter(Bytecode from, > > Bytecode to) const; > > On 2016/04/25 13:05:45, rmcilroy wrote: > > > This can be private > > > > Done. > > Just as a heads-up, tools/gyp/v8.gyp is now src/v8.gyp. Thank you! Rebased patch coming.
On 2016/04/25 14:09:52, oth wrote: > On 2016/04/25 13:24:07, Stefano Sanfilippo wrote: > > > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > > File src/interpreter/interpreter.h (right): > > > > > https://codereview.chromium.org/1899133004/diff/380001/src/interpreter/interp... > > src/interpreter/interpreter.h:52: uintptr_t GetDispatchCounter(Bytecode from, > > Bytecode to) const; > > On 2016/04/25 13:05:45, rmcilroy wrote: > > > This can be private > > > > Done. > > Just as a heads-up, tools/gyp/v8.gyp is now src/v8.gyp. Not yet, they exist both and need to stay in sync until it's changed for all embedders.
Added ignition-statistics-extension.{cc,h} to src/v8.gyp.
Is there any way I can try a build with the new script (location)?
https://codereview.chromium.org/1899133004/diff/380001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):
https://codereview.chromium.org/1899133004/diff/380001/tools/gyp/v8.gyp#newco...
tools/gyp/v8.gyp:776: '../../src/extensions/ignition-statistics-extension.cc',
On 2016/04/25 13:03:17, Michael Achenbach wrote:
> Please add this also to src/v8.gyp so that they stay in sync
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/1899133004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/420001
gyp lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/25 14:55:46, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Can you elaborate on the use case of getIgnitionDispatchCounters() exposed via extension? Do you need it in Chrome?
On 2016/04/26 13:09:52, Yang wrote: > On 2016/04/25 14:55:46, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Can you elaborate on the use case of getIgnitionDispatchCounters() exposed via > extension? Do you need it in Chrome? Yes. We wanted to do some measurements on real-world examples, but we can't just write a file from the process V8 runs in, because of sandboxing. We had a brief discussion with primiano@ of the Clank team and figured out that calling JSON.stringify(getIgnitionDispatchCounters()) is the simplest way of retrieving the dispatch counters.
On 2016/04/26 13:19:56, Stefano Sanfilippo wrote: > On 2016/04/26 13:09:52, Yang wrote: > > On 2016/04/25 14:55:46, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > Can you elaborate on the use case of getIgnitionDispatchCounters() exposed via > > extension? Do you need it in Chrome? > > Yes. > > We wanted to do some measurements on real-world examples, but we can't just > write a file from the process V8 runs in, because of sandboxing. > > We had a brief discussion with primiano@ of the Clank team and figured out that > calling JSON.stringify(getIgnitionDispatchCounters()) is the simplest way of > retrieving the dispatch counters. please add a test case for the extension. Otherwise LGTM.
On 2016/04/26 13:35:20, Yang wrote: > On 2016/04/26 13:19:56, Stefano Sanfilippo wrote: > > On 2016/04/26 13:09:52, Yang wrote: > > > On 2016/04/25 14:55:46, commit-bot: I haz the power wrote: > > > > Dry run: This issue passed the CQ dry run. > > > > > > Can you elaborate on the use case of getIgnitionDispatchCounters() exposed > via > > > extension? Do you need it in Chrome? > > > > Yes. > > > > We wanted to do some measurements on real-world examples, but we can't just > > write a file from the process V8 runs in, because of sandboxing. > > > > We had a brief discussion with primiano@ of the Clank team and figured out > that > > calling JSON.stringify(getIgnitionDispatchCounters()) is the simplest way of > > retrieving the dispatch counters. > > please add a test case for the extension. Otherwise LGTM. mjsunit test added. My JavaScript is far from perfect, I'd be grateful if someone could take a look at it.
Patchset #10 (id:460001) has been deleted
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/1899133004/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/04/26 17:18:40, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm.
mjsunit test lgtm, thanks.
The CQ bit was checked by ssanfilippo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org, machenbach@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/1899133004/#ps520001 (title: "Check that the outer object has at least one property.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899133004/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/520001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/784) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/4928) v8_mac_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/794)
Patchset #13 (id:540001) has been deleted
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/1899133004/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/560001
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 oth@chromium.org, rmcilroy@chromium.org, yangguo@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1899133004/#ps560001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1899133004/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899133004/560001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters as a JavaScript object. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters as a JavaScript object. BUG=v8:4899 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #13 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters as a JavaScript object. BUG=v8:4899 LOG=N ========== to ========== [Interpreter] Add Ignition statistics JavaScript extension. This commit introduces IgnitionStatisticsExtension, which provides methods for accessing Ignition statistics and counters from JavaScript. The extension is registered when FLAG_ignition and FLAG_trace_ignition_dispatches are both enabled. For the moment, the only exposed function is getIgnitionDispatchCounters(), which allows to retrieve Ignition dispatch counters as a JavaScript object. BUG=v8:4899 LOG=N Committed: https://crrev.com/905becd13b8696e126255decf130fdb9e1d9aa30 Cr-Commit-Position: refs/heads/master@{#35816} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/905becd13b8696e126255decf130fdb9e1d9aa30 Cr-Commit-Position: refs/heads/master@{#35816} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
