Bug 140962
| Summary: | Web Inspector: type profiler does not dim functions that were never called | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Burg <burg> |
| Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | graouts, inspector-bugzilla-changes, jonowells, saam, webkit-bug-importer |
| Priority: | P2 | Keywords: | InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Bug Depends on: | 141976 | ||
| Bug Blocks: | |||
Brian Burg
This can be misleading if you are not paying close attention.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Radar WebKit Bug Importer
<rdar://problem/19621225>
Brian Burg
In particular, a function that is never called will have the same background color as a function in which every line is executed at least once.
Saam Barati
The problem here is that when the inspector first opens with type profiling enabled (or the user turns type profiling on mid-way through inspection), JavaScriptCore has an incomplete view of those functions that have run and those functions that have not run. Because of this, JavaScriptCore won't get completely accurate information until the page is refreshed.
The approach to this problem is two fold:
1. We're going to switch the default to be from assuming a function has run to assuming it has not run. This doesn't solve the problem, but it is less misleading and probably a better default.
2. We're removing gray backgrounds of unexecuted basic blocks and instead are going to lessen the saturation of the syntax highlighting on these unexecuted blocks. Again, this doesn't solve the problem, but provides a more subtle UI that is less obviously misleading. It is also more pleasant to use.
In the future, we can solve this more directly by at least considering enabling JavaScriptCore's /runtime/FunctionHasExecutedCache class to be enabled at all time and not only when the type profiler or control flow profiler is enabled.
Blaze Burg
(In reply to comment #3)
> The problem here is that when the inspector first opens with type profiling
> enabled (or the user turns type profiling on mid-way through inspection),
> JavaScriptCore has an incomplete view of those functions that have run and
> those functions that have not run. Because of this, JavaScriptCore won't get
> completely accurate information until the page is refreshed.
>
> The approach to this problem is two fold:
>
> 1. We're going to switch the default to be from assuming a function has run
> to assuming it has not run. This doesn't solve the problem, but it is less
> misleading and probably a better default.
>
> 2. We're removing gray backgrounds of unexecuted basic blocks and instead
> are going to lessen the saturation of the syntax highlighting on these
> unexecuted blocks. Again, this doesn't solve the problem, but provides a
> more subtle UI that is less obviously misleading. It is also more pleasant
> to use.
>
> In the future, we can solve this more directly by at least considering
> enabling JavaScriptCore's /runtime/FunctionHasExecutedCache class to be
> enabled at all time and not only when the type profiler or control flow
> profiler is enabled.
What's the state of this bug today, Saam?
Saam Barati
(In reply to comment #4)
> (In reply to comment #3)
> > The problem here is that when the inspector first opens with type profiling
> > enabled (or the user turns type profiling on mid-way through inspection),
> > JavaScriptCore has an incomplete view of those functions that have run and
> > those functions that have not run. Because of this, JavaScriptCore won't get
> > completely accurate information until the page is refreshed.
> >
> > The approach to this problem is two fold:
> >
> > 1. We're going to switch the default to be from assuming a function has run
> > to assuming it has not run. This doesn't solve the problem, but it is less
> > misleading and probably a better default.
> >
> > 2. We're removing gray backgrounds of unexecuted basic blocks and instead
> > are going to lessen the saturation of the syntax highlighting on these
> > unexecuted blocks. Again, this doesn't solve the problem, but provides a
> > more subtle UI that is less obviously misleading. It is also more pleasant
> > to use.
> >
> > In the future, we can solve this more directly by at least considering
> > enabling JavaScriptCore's /runtime/FunctionHasExecutedCache class to be
> > enabled at all time and not only when the type profiler or control flow
> > profiler is enabled.
>
> What's the state of this bug today, Saam?
So we implemented (2) a while ago.
I tried implementing (1) and unfortunately there is a dependency between
the way the control flow profiler works and the way we display the UI.
Basically, the control flow profiler is broken for programs with very
weird control flow and how that interacts with how we generate control flow
in JSC's bytecode. It's probably worth fixing this. I'm not sure what the solution
is. I remember looking into this a year ago but I gave up at some point
because I kept running into bugs. I'd be happy to look at this again at some point.
It probably requires completely rewriting the control flow profiler to be
tolerant of how we emit code in bytecode not aligning well. Maybe we should
rewrite it to be an analysis over bytecode basic blocks and then correlate
the bytecode basic blocks back to basic blocks in the JS program.