Profile dynamic calls.

Profile dynamic calls.


Debug logic

BUG=

patch from issue 2012423005 at patchset 1 (http://crrev.com/2012423005#ps1)

R=jacobr@google.com, leafp@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/682a73eff224c788415235016113b59426a91d88

[priscillalee, 2016-06-01 01:13:40]

priscillalee@google.com changed reviewers:
+ jacobr@google.com

[Jacob, 2016-06-01 01:23:17]

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:165: Map<String, int>
callMethodStats = new Map();
this one is my fault but fix it anyway :)
change this to _callMethodStats.
You should be able to use the refactor support in atom to fix all occurences.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:167: class ProfileEntry {
you can make all fields in this class final.
E.g.
final String key;
instead of 
String key.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:170: num total_pct;
See the dart style guide for naming members.
https://www.dartlang.org/effective-dart/style/

for example this member could be totalPercentage
instead of total_pct.
For this particular case we should actually just remove total_pct and
total_pct_div_n 
as they can be computed easily enough in the client application so we don't need
to include them here.
Btw, as a follow up cl I expect we will add more methods to this class such as
lists of the encountered types, etc so having a class for this is worthwhile.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:179: //sb.write("Stats:");
remove commented out lines.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:181: num total = 0;
you can remove total now that we are calculating it in a separate UI.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:205: bool _track_profile =
false;
this should be named _trackProfile.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:208: _track_profile = b;
nit: change this from bool v
to 
bool value

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:217: String stackStr =
JS('String', "new Error().stack");
add
// TODO(jacobr): pass caller information in instead of using the stack trace.

[Jacob, 2016-06-01 17:13:31]

Description was changed from

==========
Profile dynamic calls.


Debug logic

BUG=

patch from issue 2012423005 at patchset 1 (http://crrev.com/2012423005#ps1)
==========

to

==========
Profile dynamic calls.


Debug logic

BUG=

patch from issue 2012423005 at patchset 1 (http://crrev.com/2012423005#ps1)
==========

[priscillalee, 2016-06-01 20:38:09]

Made the suggested revisions.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:165: Map<String, int>
callMethodStats = new Map();
On 2016/06/01 01:23:17, Jacob wrote:
> this one is my fault but fix it anyway :)
> change this to _callMethodStats.
> You should be able to use the refactor support in atom to fix all occurences.

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:167: class ProfileEntry {
On 2016/06/01 01:23:17, Jacob wrote:
> you can make all fields in this class final.
> E.g.
> final String key;
> instead of 
> String key.

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:170: num total_pct;
On 2016/06/01 01:23:17, Jacob wrote:
> See the dart style guide for naming members.
> https://www.dartlang.org/effective-dart/style/
> 
> for example this member could be totalPercentage
> instead of total_pct.
> For this particular case we should actually just remove total_pct and
> total_pct_div_n 
> as they can be computed easily enough in the client application so we don't
need
> to include them here.
> Btw, as a follow up cl I expect we will add more methods to this class such as
> lists of the encountered types, etc so having a class for this is worthwhile.

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:179: //sb.write("Stats:");
On 2016/06/01 01:23:17, Jacob wrote:
> remove commented out lines.

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:181: num total = 0;
On 2016/06/01 01:23:17, Jacob wrote:
> you can remove total now that we are calculating it in a separate UI.

thanks, I also removed sum.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:205: bool _track_profile =
false;
On 2016/06/01 01:23:17, Jacob wrote:
> this should be named _trackProfile.

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:208: _track_profile = b;
On 2016/06/01 01:23:17, Jacob wrote:
> nit: change this from bool v
> to 
> bool value

Acknowledged.

https://codereview.chromium.org/2024353002/diff/1/tool/input_sdk/private/ddc_...
tool/input_sdk/private/ddc_runtime/operations.dart:217: String stackStr =
JS('String', "new Error().stack");
On 2016/06/01 01:23:17, Jacob wrote:
> add
> // TODO(jacobr): pass caller information in instead of using the stack trace.

Acknowledged.

[Jacob, 2016-06-01 20:48:20]

jacobr@google.com changed reviewers:
+ leafp@google.com

[Jacob, 2016-06-01 20:48:20]

lgtm from my perspective.
Added a couple more small style comments I'd missed before.
Adding leafp to evaluate the cl from a performance perspective.
The main question is whether we need to optimize this further or whether dcalls
are already slow enough that adding an extra function call that quickly short
circuits is fine.

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:174: dumpDynamicStats() {
rename dumpDynamicStats()
to 
List<ProfileEntry> getDynamicStats()

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:206: String src = '';
nit: change String to var

[priscillalee, 2016-06-01 21:03:33]

Made a few style revisions.

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:174: dumpDynamicStats() {
On 2016/06/01 20:48:20, Jacob wrote:
> rename dumpDynamicStats()
> to 
> List<ProfileEntry> getDynamicStats()

Done.

https://codereview.chromium.org/2024353002/diff/20001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:206: String src = '';
On 2016/06/01 20:48:20, Jacob wrote:
> nit: change String to var

Done.

[Jacob, 2016-06-01 21:38:58]

lgtm

[Leaf, 2016-06-01 21:47:11]

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:35: $_trackCall(obj, $field,
null);
Why is tracking for this done on the canonical name but the rest use the
non-canonical name?

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:94: $_trackCall(obj, name,
f.toString());
I think this double counts, since callMethod uses this as a helper.

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:201: if (!_trackProfile)
return;
Can we make this inline JS to avoid the pointless dart.test in the generated
code?

[priscillalee, 2016-06-01 23:32:15]

Fixed suggested revisions.

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:35: $_trackCall(obj, $field,
null);
On 2016/06/01 21:47:11, Leaf wrote:
> Why is tracking for this done on the canonical name but the rest use the
> non-canonical name?

Thanks for catching that. Fixed tracking to be on canonical name in all cases.

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:94: $_trackCall(obj, name,
f.toString());
On 2016/06/01 21:47:11, Leaf wrote:
> I think this double counts, since callMethod uses this as a helper.

Removed _trackCall from _callMethod, so there's no more double counting.

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:201: if (!_trackProfile)
return;
On 2016/06/01 21:47:11, Leaf wrote:
> Can we make this inline JS to avoid the pointless dart.test in the generated
> code?

Done.

[Leaf, 2016-06-01 23:59:43]

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:94: $_trackCall(obj, name,
f.toString());
I missed this the first time, but the f.toString() shouldn't be executed when
we're not tracking.  It looks like it's unused below, so maybe you can just get
rid of it. Otherwise maybe we should consider hoisting the trackProfile check up
to the call site, something like:

  $_trackProfile && $_trackCall(obj, name, f.toString());

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:200: _trackCall(obj, name,
displayName) {
Is displayName unused?

[Jacob, 2016-06-02 00:10:28]

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:200: _trackCall(obj, name,
displayName) {
On 2016/06/01 23:59:43, Leaf wrote:
> Is displayName unused?

It is unused.

[priscillalee, 2016-06-02 16:19:48]

Removed displayName and f.toString() from _trackCall.

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
File tool/input_sdk/private/ddc_runtime/operations.dart (right):

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:94: $_trackCall(obj, name,
f.toString());
On 2016/06/01 23:59:43, Leaf wrote:
> I missed this the first time, but the f.toString() shouldn't be executed when
> we're not tracking.  It looks like it's unused below, so maybe you can just
get
> rid of it. Otherwise maybe we should consider hoisting the trackProfile check
up
> to the call site, something like:
> 
>   $_trackProfile && $_trackCall(obj, name, f.toString());
> 

Removed f.toString().

https://codereview.chromium.org/2024353002/diff/40001/tool/input_sdk/private/...
tool/input_sdk/private/ddc_runtime/operations.dart:200: _trackCall(obj, name,
displayName) {
On 2016/06/02 00:10:28, Jacob wrote:
> On 2016/06/01 23:59:43, Leaf wrote:
> > Is displayName unused?
> 
> It is unused.

Removed displayName.

[Jacob, 2016-06-02 16:24:32]

lgtm

[Leaf, 2016-06-02 16:32:15]

lgtm

[Jacob, 2016-06-02 16:59:27]

Description was changed from

==========
Profile dynamic calls.


Debug logic

BUG=

patch from issue 2012423005 at patchset 1 (http://crrev.com/2012423005#ps1)
==========

to

==========
Profile dynamic calls.


Debug logic

BUG=

patch from issue 2012423005 at patchset 1 (http://crrev.com/2012423005#ps1)

R=jacobr@google.com, leafp@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/682a73eff224c788415235016113...
==========

[Jacob, 2016-06-02 16:59:29]

Committed patchset #6 (id:100001) manually as
682a73eff224c788415235016113b59426a91d88 (presubmit successful).