* 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.
<rdar://problem/23700835>
Created attachment 284830 [details] WIP
Created attachment 284831 [details] WIP
(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.
(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.
(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.
(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.
(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!
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.
(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.
(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.
(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.