Dev compiler debugger related tweaks.

Dev compiler debugger related tweaks.

Optimize dynamic call tracking using random sampling.
Apply source maps to improve dynamic call tracking accuracy.
Dynamic calls within the sdk are now attributed correctly.

Fix bug formatting modules where we were incorrectly flagging them as JS.

BUG=
R=jmesserly@google.com, vsm@google.com

Committed: https://github.com/dart-lang/sdk/commit/b280a7803df850e13880925bb35b092fb933ba39

[Jacob, 2017-04-12 18:24:41]

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

[Jacob, 2017-04-12 18:24:43]

https://codereview.chromium.org/2811343002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2811343002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:611: return
'dart:${url.substring(urlPrefix.length)}';
Users couldn't access these source files anyway so might as well reference them
by
dart:
names instead of the confusing names we were using.

https://codereview.chromium.org/2811343002/diff/10001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2811343002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:367: double
count;
Switch to a a random sampling profiler to make profiling less horribly slow.
Users should only worry about common dynamic calls rather than trying to
eliminate all dynamism from their programs.

https://codereview.chromium.org/2811343002/diff/10001/pkg/dev_compiler/web/ma...
File pkg/dev_compiler/web/main.dart (right):

https://codereview.chromium.org/2811343002/diff/10001/pkg/dev_compiler/web/ma...
pkg/dev_compiler/web/main.dart:26: setUpCompilerInBrowser = allowInterop((String
sdkUrl,
Remove summaryRoot parameter as it is obsolete and possibly wrong if summaries
are loaded from two different Dart applications on the same page perhaps with
different summary root directories.

https://codereview.chromium.org/2811343002/diff/30001/tests/lib_strong/html/d...
File tests/lib_strong/html/debugger_test_golden.txt (right):

https://codereview.chromium.org/2811343002/diff/30001/tests/lib_strong/html/d...
tests/lib_strong/html/debugger_test_golden.txt:5897: "Library Module:
lib/html/debugger_test"
Correctly identified this was a Dart Library module instead of a JS object.

[vsm, 2017-04-12 21:40:47]

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:609: var urlPrefix =
'../../../gen/patched_sdk/lib/';
Is there a cleaner way to get this?  It's likely to change when we build sdk as
part of the build.

Though I guess this will throw when it does.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:383: ///
Size for the random sample of dynamic calls.
this is perhaps big enough to move out to a separate file?  e.g., profile.dart

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:421: //
Truncate the stacktrace as we are only interested in the top 3-4 frames.
nit: line len

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:430:
'dart:_internal/js_runtime/lib/ddc_runtime/operations.dart')) {
this is a weird uri - are we replacing some prefix with 'dart:' somewhere?

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:448: if
(stats.count + 0.001 < threshold) {
why the 0.001?

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:490: }
Interesting sampling strategy!  Looks like entries will be somewhat weighted by
time.  E.g., last one wins, right?

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/update.sh
File pkg/dev_compiler/update.sh (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/update...
pkg/dev_compiler/update.sh:3: cp tool/input_sdk/private/ddc_runtime/*.dart
/google/src/cloud/jacobr/land_debugger/google3/third_party/dart_lang/$DART_VERSION/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/
did you mean to include this file?

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
File pkg/dev_compiler/web/web_command.dart (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
pkg/dev_compiler/web/web_command.dart:100: var summaryResponses;
Type var to avoid dcall below.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
pkg/dev_compiler/web/web_command.dart:112: print("Summaries loaded");
Where do prints go for the web compiler?

[Jacob, 2017-04-12 23:42:10]

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/compiler.dart:609: var urlPrefix =
'../../../gen/patched_sdk/lib/';
On 2017/04/12 21:40:47, vsm wrote:
> Is there a cleaner way to get this?  It's likely to change when we build sdk
as
> part of the build.
> 
> Though I guess this will throw when it does.

Yeah. If we change the directory structure this will throw and we can add the
new path. Other option is we just guess that the url contains
"/patched_sdk/lib/"
which in practice is unlikely to have false positives and probably won't have
to change when the sdk is refactored.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:383: ///
Size for the random sample of dynamic calls.
On 2017/04/12 21:40:46, vsm wrote:
> this is perhaps big enough to move out to a separate file?  e.g., profile.dart

Agreed. I was feeling bad about this polluting operations.dart
Moved to profile.dart

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:421: //
Truncate the stacktrace as we are only interested in the top 3-4 frames.
On 2017/04/12 21:40:47, vsm wrote:
> nit: line len
Done. Fixed comment.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:430:
'dart:_internal/js_runtime/lib/ddc_runtime/operations.dart')) {
On 2017/04/12 21:40:47, vsm wrote:
> this is a weird uri - are we replacing some prefix with 'dart:' somewhere?

Yeah these are wrong. I need to use libraries.dart to fix these up. Will take a
look at that tomorrow.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:448: if
(stats.count + 0.001 < threshold) {
On 2017/04/12 21:40:46, vsm wrote:
> why the 0.001?

epsilon to avoid rounding issues. done.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:490: }
On 2017/04/12 21:40:47, vsm wrote:
> Interesting sampling strategy!  Looks like entries will be somewhat weighted
by
> time.  E.g., last one wins, right?
Last one doesn't win because we randomly choose to not include each entry in the
sample with the appropriate probability to keep the process fair.

Note this algorithm is conceptually equivalent to shuffling a list of M elements
and then selecting the first N elements of the shuffle.
The only enhancement is I don't bother shuffling until we have > N elements as I
don't care what order those elements occur in as it does not impact the rest of
the algorithm. I omitted that step to make the code simpler to write not because
of the small performance boost.
I'm referring to the standard algorithm to shuffle a list of M elements of
starting with the list in order and then for each index I from 0 to M, randomly
swap that element with the element at 0 <= I with equal probability. My code is
the same as that algorithm except for not bothering to do anything until we are
at element N + 1 and discarding all elements we know will end up at a position >
N.

For fun, here's a slightly hand-waving proof by induction that this approach is
correct:
Base case:
totalCallRecords <= _callRecordSampleSize
Everything is included... so valid "random" sample including everything.

Induction step:
Assume we have a random N record sample of M total records. Each of the original
M records has a probably N/M of being in the sample.

We add record N+1 to the sample with probability N/(M + 1) and do not include it
in the random sample the rest of the time. N / (M + 1) is the correct
probability for an element in the sample of N / (M + 1) elements.
We uniformly randomly select which of the existing N records in the sample to
remove so all records in the existing sample are removed with equal probability
thus their distribution is not skewed. As the new record has the correct
probability of being in the sample, all existing records must have the correct
probability as well although we could calculate it.
Each of the existing records had a probability N/M of being in the sample before
adding element M+1
Each of the existing records has a probability 1 / (M + 1)
of being removed as part of considering element M+1
thus its probability of being in the sample is
(N/M) * (1 - (1/(M+1)) = (N/M) * (M/(M+1)) = N/(M+1)
which is exactly what it should be.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/update.sh
File pkg/dev_compiler/update.sh (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/update...
pkg/dev_compiler/update.sh:3: cp tool/input_sdk/private/ddc_runtime/*.dart
/google/src/cloud/jacobr/land_debugger/google3/third_party/dart_lang/$DART_VERSION/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/
On 2017/04/12 21:40:47, vsm wrote:
> did you mean to include this file?

Nope... Removed.

[vsm, 2017-04-13 18:17:50]

lgtm

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:490: }
On 2017/04/12 23:42:09, Jacob wrote:
> On 2017/04/12 21:40:47, vsm wrote:
> > Interesting sampling strategy!  Looks like entries will be somewhat weighted
> by
> > time.  E.g., last one wins, right?
> Last one doesn't win because we randomly choose to not include each entry in
the
> sample with the appropriate probability to keep the process fair.
> 
> Note this algorithm is conceptually equivalent to shuffling a list of M
elements
> and then selecting the first N elements of the shuffle.
> The only enhancement is I don't bother shuffling until we have > N elements as
I
> don't care what order those elements occur in as it does not impact the rest
of
> the algorithm. I omitted that step to make the code simpler to write not
because
> of the small performance boost.
> I'm referring to the standard algorithm to shuffle a list of M elements of
> starting with the list in order and then for each index I from 0 to M,
randomly
> swap that element with the element at 0 <= I with equal probability. My code
is
> the same as that algorithm except for not bothering to do anything until we
are
> at element N + 1 and discarding all elements we know will end up at a position
>
> N.
> 
> For fun, here's a slightly hand-waving proof by induction that this approach
is
> correct:
> Base case:
> totalCallRecords <= _callRecordSampleSize
> Everything is included... so valid "random" sample including everything.
> 
> Induction step:
> Assume we have a random N record sample of M total records. Each of the
original
> M records has a probably N/M of being in the sample.
> 
> We add record N+1 to the sample with probability N/(M + 1) and do not include
it
> in the random sample the rest of the time. N / (M + 1) is the correct
> probability for an element in the sample of N / (M + 1) elements.
> We uniformly randomly select which of the existing N records in the sample to
> remove so all records in the existing sample are removed with equal
probability
> thus their distribution is not skewed. As the new record has the correct
> probability of being in the sample, all existing records must have the correct
> probability as well although we could calculate it.
> Each of the existing records had a probability N/M of being in the sample
before
> adding element M+1
> Each of the existing records has a probability 1 / (M + 1)
> of being removed as part of considering element M+1
> thus its probability of being in the sample is
> (N/M) * (1 - (1/(M+1)) = (N/M) * (M/(M+1)) = N/(M+1)
> which is exactly what it should be.

Clever!  And makes sense to me.  

Worth a comment here that this approach ensures that each sample has an equal
_callRecordSampleSize/_totalCallRecords chance of inclusion.

[Jacob, 2017-04-15 02:26:49]

Added comment indicating why the sample is random.

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
File pkg/dev_compiler/web/web_command.dart (right):

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
pkg/dev_compiler/web/web_command.dart:100: var summaryResponses;
On 2017/04/12 21:40:47, vsm wrote:
> Type var to avoid dcall below.

fixed

https://codereview.chromium.org/2811343002/diff/40001/pkg/dev_compiler/web/we...
pkg/dev_compiler/web/web_command.dart:112: print("Summaries loaded");
On 2017/04/12 21:40:47, vsm wrote:
> Where do prints go for the web compiler?
removed

https://codereview.chromium.org/2811343002/diff/90001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (left):

https://codereview.chromium.org/2811343002/diff/90001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:54: }
Jenny please review this change. This switches to showing dart: uris for all
system library files instead
of the confusing paths to their locations in the gen directory.

[Jacob, 2017-04-15 02:28:31]

Description was changed from

==========
Dev compiler debugger related tweaks.

Optimize dynamic call tracking using random sampling.
Apply source maps to improve dynamic call tracking accuracy.
Dynamic calls within the sdk are now attributed correctly.

Fix bug formatting modules where we were incorrectly flagging them as JS.

BUG=
==========

to

==========
Dev compiler debugger related tweaks.

Optimize dynamic call tracking using random sampling.
Apply source maps to improve dynamic call tracking accuracy.
Dynamic calls within the sdk are now attributed correctly.

Fix bug formatting modules where we were incorrectly flagging them as JS.

BUG=
==========

[Jacob, 2017-04-15 02:28:32]

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

[Jennifer Messerly, 2017-04-17 21:37:05]

that change lgtm!

https://codereview.chromium.org/2811343002/diff/90001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/source_map_printer.dart (left):

https://codereview.chromium.org/2811343002/diff/90001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/source_map_printer.dart:54: }
On 2017/04/15 02:26:49, Jacob wrote:
> Jenny please review this change. This switches to showing dart: uris for all
> system library files instead
> of the confusing paths to their locations in the gen directory.

Nice!!!

[Jacob, 2017-04-17 21:57:39]

Description was changed from

==========
Dev compiler debugger related tweaks.

Optimize dynamic call tracking using random sampling.
Apply source maps to improve dynamic call tracking accuracy.
Dynamic calls within the sdk are now attributed correctly.

Fix bug formatting modules where we were incorrectly flagging them as JS.

BUG=
==========

to

==========
Dev compiler debugger related tweaks.

Optimize dynamic call tracking using random sampling.
Apply source maps to improve dynamic call tracking accuracy.
Dynamic calls within the sdk are now attributed correctly.

Fix bug formatting modules where we were incorrectly flagging them as JS.

BUG=
R=jmesserly@google.com, vsm@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b280a7803df850e13880925bb35b092fb933ba39
==========

[Jacob, 2017-04-17 21:57:40]

Committed patchset #10 (id:90001) manually as
b280a7803df850e13880925bb35b092fb933ba39 (presubmit successful).