Bug 151695 – Web Inspector: Split code coverage and type profilers

• Status: ASSIGNED
• Component: Web Inspector

[Nikita Vasilyev]

* Split code coverage and type profilers;
* Turn on code coverage profiler (grayed out text) by default;
* Turn off type profiler (type info pills) by default;
* [T] button will continue to toggle type profiler but won't affect code coverage profiler;
* Code hotness visualizer, when implemented, will introduce a new button next to [T] and will be off by default.

[Radar WebKit Bug Importer]

<rdar://problem/23700835>

[Nikita Vasilyev]

Created attachment 284830 [details]
WIP

[Nikita Vasilyev]

Created attachment 284831 [details]
WIP

[Blaze Burg]

(In reply to comment #0)
> * Split code coverage and type profilers;
> * Turn on code coverage profiler (grayed out text) by default;
> * Turn off type profiler (type info pills) by default;
> * [T] button will continue to toggle type profiler but won't affect code
> coverage profiler;
> * Code hotness visualizer, when implemented, will introduce a new button
> next to [T] and will be off by default.

Please do not do all of these things in one patch.

[Joseph Pecoraro]

(In reply to comment #0)
> * Turn on code coverage profiler (grayed out text) by default;

When doing this, it would be good to understand its performance impact. We spent a lot of time optimizing performance when the inspector is open for Timelines. It would be good to understand what the impact of this is if we have it always on. Or, change Timeline recording to also turn this off, to avoid a performance impact.

[Nikita Vasilyev]

(In reply to comment #5)
> (In reply to comment #0)
> > * Turn on code coverage profiler (grayed out text) by default;
> 
> When doing this, it would be good to understand its performance impact. We
> spent a lot of time optimizing performance when the inspector is open for
> Timelines. It would be good to understand what the impact of this is if we
> have it always on. Or, change Timeline recording to also turn this off, to
> avoid a performance impact.

The front-end doesn't do anything performance intensive until a JS resource is visible.

I don't know what happens on the backend. Saam should know.

[Nikita Vasilyev]

(In reply to comment #4)
> (In reply to comment #0)
> > * Split code coverage and type profilers;
> > * Turn on code coverage profiler (grayed out text) by default;
> > * Turn off type profiler (type info pills) by default;
> > * [T] button will continue to toggle type profiler but won't affect code
> > coverage profiler;
> > * Code hotness visualizer, when implemented, will introduce a new button
> > next to [T] and will be off by default.
> 
> Please do not do all of these things in one patch.

I intended to do the first 4 points in one patch.
2nd to 4th points are just clarifications for the 1st.

[Joseph Pecoraro]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #0)
> > > * Turn on code coverage profiler (grayed out text) by default;
> > 
> > When doing this, it would be good to understand its performance impact. We
> > spent a lot of time optimizing performance when the inspector is open for
> > Timelines. It would be good to understand what the impact of this is if we
> > have it always on. Or, change Timeline recording to also turn this off, to
> > avoid a performance impact.
> 
> The front-end doesn't do anything performance intensive until a JS resource
> is visible.
> 
> I don't know what happens on the backend. Saam should know.

I'm worried about how this will slow down content on the page. For example, running a benchmark with the code coverage enabled / not enabled. Anyone can run that and get numbers. It may be negligible, but we don't know until we measure. And we can't easily until this is separated from the Type Profiler, which you are doing!

[Nikita Vasilyev]

Saam,

RuntimeAgent.enableTypeProfiler() currently enables both type profiler AND basic blocks annotator (for grayed out unexecuted code).

How hard would it be to decouple these two?

Currently, calling RuntimeAgent.getBasicBlocks before RuntimeAgent.enableTypeProfiler() results in error:
    [Error] Error in getting basic block locations: The VM does not currently have a Control Flow Profiler.

[Saam Barati]

(In reply to comment #0)
> * Split code coverage and type profilers;
This should be easy to do. It probably requires hacking
on the protocol a bit, but the control flow profiler, and type profiler,
are logically separate inside JSC. I can help review such a patch. I agree
with Brian that this should be done in its own patch (I have less preference on
the combining of other bits in other patches because I don't work on Inspector much).

> * Turn on code coverage profiler (grayed out text) by default;
This sounds like a good idea to me. The performance impact of control flow
profiler was something like 10% last time I checked. Maybe a bit less.
But regardless, as Joe mentioned, this should definitely be disabled when
recording a timeline. We don't want this influencing the data we get out
of the sampling profiler.

> * Turn off type profiler (type info pills) by default;
Isn't this already the default?

> * [T] button will continue to toggle type profiler but won't affect code
> coverage profiler;
Sounds good to me.

> * Code hotness visualizer, when implemented, will introduce a new button
> next to [T] and will be off by default.
What is the code hotness visualizer? Is this going to use data from the sampling
profiler? Were you thinking this should be a hybrid of the sampling profiler and
the control flow profiler? I think it's really important that it's not a hybrid.
We don't want the control flow profiler to be enabled when sampling because it
will bias the data we get from the sample. We worked really hard to have the inspector
disable other JSC profilers/debugger features while taking a sample because we
want JSC to run the program being sampled in as close to its natural state as possible.

I guess if it's not a hybrid of these two profilers, it's important to ask if we really
need to decouple this from the type profiler. It seems like there is both good and bad
in doing this. It's nice to have the control flow profiler always on. It's bad to add complexity
to the inspector's interface by allowing them to be decoupled. Others probably have stronger/more
informed opinions about this than me.

[Nikita Vasilyev]

(In reply to comment #10)
> (In reply to comment #0)
> > * Split code coverage and type profilers;
> This should be easy to do. It probably requires hacking
> on the protocol a bit, but the control flow profiler, and type profiler,
> are logically separate inside JSC. I can help review such a patch. I agree
> with Brian that this should be done in its own patch (I have less preference
> on
> the combining of other bits in other patches because I don't work on
> Inspector much).

I agree now that backend (JSC) and Web Inspector front-end changes should be
in separate patches.

> 
> > * Turn on code coverage profiler (grayed out text) by default;
> This sounds like a good idea to me. The performance impact of control flow
> profiler was something like 10% last time I checked. Maybe a bit less.
> But regardless, as Joe mentioned, this should definitely be disabled when
> recording a timeline. We don't want this influencing the data we get out
> of the sampling profiler.

Yes, it should be disabled when recording a timeline.

> 
> > * Turn off type profiler (type info pills) by default;
> Isn't this already the default?

Yes, it is. I meant to write "Keep type profiler turned off by default".

> > * Code hotness visualizer, when implemented, will introduce a new button
> > next to [T] and will be off by default.
> What is the code hotness visualizer? Is this going to use data from the
> sampling
> profiler? Were you thinking this should be a hybrid of the sampling profiler
> and
> the control flow profiler? I think it's really important that it's not a
> hybrid.
> We don't want the control flow profiler to be enabled when sampling because
> it
> will bias the data we get from the sample. We worked really hard to have the
> inspector
> disable other JSC profilers/debugger features while taking a sample because
> we
> want JSC to run the program being sampled in as close to its natural state
> as possible.

I had written this before sampling profiler was implemented.
Let's keep the code hotness visualizer, whatever it would be, out of the scope of this bug.

> 
> I guess if it's not a hybrid of these two profilers, it's important to ask
> if we really
> need to decouple this from the type profiler. It seems like there is both
> good and bad
> in doing this. It's nice to have the control flow profiler always on. It's
> bad to add complexity
> to the inspector's interface by allowing them to be decoupled. Others
> probably have stronger/more
> informed opinions about this than me.

I find type profiler too intrusive to keep it enabled all the time. 
The control flow profiler adds a lot of value and doesn't
take any extra screen space.

[Joseph Pecoraro]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #0)
> > > * Split code coverage and type profilers;
> > [...] I agree with Brian that this should be done in its own patch
> > (I have less preference on the combining of other bits in other
> > patches because I don't work on Inspector much).
> 
> I agree now that backend (JSC) and Web Inspector front-end changes should be
> in separate patches.

This is not so much about separate patches for backend and frontend changes. This is about separate patches for separate work. Splitting the two profilers is a single task, and that task should have its own bug and patch, separate from all the other tasks mentioned in this bug.