Support narrowing a cpu profile to a given time window

Support narrowing a cpu profile to a given time window

- Add optional 'timeOriginMicros' and 'timeExtentMicros' parameters to all CPU profile service protocol RPCs.
- Add test to verify that time filtering works.

R=asiva@google.com

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

[Cutch, 2015-12-21 18:23:14]

Description was changed from

==========
Support getting a cpu profile within a given time window

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

to

==========
Support getting a cpu profile within a given time window

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

[Cutch, 2015-12-21 18:23:14]

johnmccutchan@google.com changed reviewers:
+ asiva@google.com

[Cutch, 2015-12-21 18:24:00]

Description was changed from

==========
Support getting a cpu profile within a given time window

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

to

==========
Support narrowing a cpu profile to a given time window

- Add optional 'timeOriginMicros' and 'timeExtentMicros' parameters to all CPU
profile service protocol RPCs.
- Add test to verify that time filtering works.
==========

[siva, 2015-12-21 20:43:28]

lgtm

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:96: int64_t time_extent_micros = -1)
I don't see any place where SampleFilter is constructed with just one argument
passed in, why not make time_origin_micros and time_extent_micros regular params
without any default values?

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:99: time_extent_micros_(time_extent_micros) { }
ASSERT(time_origin_micros >= 0);
ASSERT(time_extent_micros >= 0);

because of the math done in the implementation of TimeFilterSample

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler_test.cc
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:152: explicit AllocationFilter(Isolate* isolate,
explicit not needed, it has two mandatory arguments.

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:500: }
if (*value == '\0') should we also return the default_value?

[Cutch, 2015-12-21 22:07:38]

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:96: int64_t time_extent_micros = -1)
On 2015/12/21 20:43:27, siva wrote:
> I don't see any place where SampleFilter is constructed with just one argument
> passed in, why not make time_origin_micros and time_extent_micros regular
params
> without any default values?

Done.

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:99: time_extent_micros_(time_extent_micros) { }
On 2015/12/21 20:43:28, siva wrote:
> ASSERT(time_origin_micros >= 0);
> ASSERT(time_extent_micros >= 0);
> 
> because of the math done in the implementation of TimeFilterSample

When these are -1 it means we don't have a TimeFilterSample.

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler_test.cc
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler_test.cc...
runtime/vm/profiler_test.cc:152: explicit AllocationFilter(Isolate* isolate,
On 2015/12/21 20:43:28, siva wrote:
> explicit not needed, it has two mandatory arguments.

Done.

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/service.cc#newco...
runtime/vm/service.cc:500: }
On 2015/12/21 20:43:28, siva wrote:
> if (*value == '\0') should we also return the default_value?

Done.

[Cutch, 2015-12-21 22:07:52]

Description was changed from

==========
Support narrowing a cpu profile to a given time window

- Add optional 'timeOriginMicros' and 'timeExtentMicros' parameters to all CPU
profile service protocol RPCs.
- Add test to verify that time filtering works.
==========

to

==========
Support narrowing a cpu profile to a given time window

- Add optional 'timeOriginMicros' and 'timeExtentMicros' parameters to all CPU
profile service protocol RPCs.
- Add test to verify that time filtering works.

R=asiva@google.com

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

[Cutch, 2015-12-21 22:07:53]

Committed patchset #3 (id:40001) manually as
d187d4745e5d2acd62d114092d3456672a58d030 (presubmit successful).

[siva, 2015-12-22 17:08:23]

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
runtime/vm/profiler.h:99: time_extent_micros_(time_extent_micros) { }
On 2015/12/21 22:07:38, Cutch wrote:
> On 2015/12/21 20:43:28, siva wrote:
> > ASSERT(time_origin_micros >= 0);
> > ASSERT(time_extent_micros >= 0);
> > 
> > because of the math done in the implementation of TimeFilterSample
> 
> When these are -1 it means we don't have a TimeFilterSample.

Sorry I meant :
ASSERT(time_origin_micros >= -1);
ASSERT(time_extent_micros >= -1);

[Cutch, 2015-12-22 17:14:51]

On 2015/12/22 17:08:23, siva wrote:
> https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h
> File runtime/vm/profiler.h (right):
> 
>
https://codereview.chromium.org/1541893002/diff/1/runtime/vm/profiler.h#newco...
> runtime/vm/profiler.h:99: time_extent_micros_(time_extent_micros) { }
> On 2015/12/21 22:07:38, Cutch wrote:
> > On 2015/12/21 20:43:28, siva wrote:
> > > ASSERT(time_origin_micros >= 0);
> > > ASSERT(time_extent_micros >= 0);
> > > 
> > > because of the math done in the implementation of TimeFilterSample
> > 
> > When these are -1 it means we don't have a TimeFilterSample.
> 
> Sorry I meant :
> ASSERT(time_origin_micros >= -1);
> ASSERT(time_extent_micros >= -1);

Done in https://codereview.chromium.org/1533323004/