Assembly view part 1#4478
Conversation
Codecov ReportBase: 88.63% // Head: 88.64% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4478 +/- ##
=======================================
Coverage 88.63% 88.64%
=======================================
Files 284 284
Lines 25532 25587 +55
Branches 6865 6882 +17
=======================================
+ Hits 22630 22681 +51
- Misses 2695 2701 +6
+ Partials 207 205 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This makes me think that we should try to have a way to map this back to where the label frame is inserted and show the source code. |
That would be ideal, but I'm not sure how to make it work. Storing information about file + line number in the profiling stack would require additional space and additional instructions per push/pop, and I worry that it might penalize WebIDL binding performance and code size. |
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
Thanks for this work, this looks pretty solid!
I have a few comments related to "where this code should go", a few questions, and a few nits (some of them optional).
I'm approving so that you can land this after handling these.
Please ping me if you have questions!
| // When this is incremented, the assembly view scrolls to the "hotspot" line. | ||
| activationGeneration: number, |
There was a problem hiding this comment.
Then, should that be called scrollGeneration, if this controls the scroll operation?
There was a problem hiding this comment.
I'll fix this in a separate patch because the source view does the same.
| * the frame table, and adding one byte (because the instruction at that address | ||
| * is at least one byte long). |
There was a problem hiding this comment.
This is probably a very naive quesiton:
you write "at least one byte long", so this means that this can be longer, right?
In that case, shouldn't we take the largest possible size for an instruction instead?
There was a problem hiding this comment.
It could be longer, but this function computes a lower bound for the function size, so we should take the smallest possible size. Otherwise we might disassemble instructions from the next function by accident. We don't want to overrun.
| * cases, a call node can have its native code in multiple different functions, | ||
| * for example if it was inlined into multiple different functions. | ||
| * Another way to get multiple native symbols here is if a call tree transform | ||
| * collapsed frames from different original functions into a merged function. |
There was a problem hiding this comment.
Is the inverted call tree another example (like mentioned elsewhere)?
There was a problem hiding this comment.
Yes, I'll adjust the sentence about "inlined into multiple different functions" to mention the inverted tree.
|
|
||
| const nativeSymbolSet = new Set( | ||
| [ | ||
| ...getNativeSymbolsForCallNode( |
There was a problem hiding this comment.
Can the result of getNativeSymbolsForCallNode have a lot of items? My understanding is that this is very unlikely, but could that happen for inverted trees for example? In that case would it make sense to iterate the Set with (for example) a for .. of and create the new one with set.add?
On another hand, it doesn't look like that having a Set for nativeSymbolSet is useful since it contains objects. What do you think of using an Array for this data?
There was a problem hiding this comment.
In the vast majority of cases it will be 1 element, sometimes 2 or 3, and probably never more than 100 (though you could create a synthetic testcase with an arbitrary amount).
Using an array sounds good to me!
| @@ -145,6 +145,7 @@ const panelLayoutGeneration: Reducer<number> = (state = 0, action) => { | |||
| case 'POP_COMMITTED_RANGES': | |||
| // Bottom box: (fallthrough) | |||
| case 'OPEN_SOURCE_VIEW': | |||
There was a problem hiding this comment.
nit: this OPEN_SOURCE_VIEW was forgotten
| transforms: TransformStacksPerThread, | ||
| timelineType: TimelineType, | ||
| sourceView: SourceViewState, | ||
| assemblyView: AssemblyViewState, |
There was a problem hiding this comment.
I see that his is not (un/)serialized in the url-handling code, do you intend to serialize this data later? If not, it might make sense to move it to another state, maybe AppState?
There was a problem hiding this comment.
I do intend to serialize this data later. I have the patch (it's in the full series in the other PR) but I wanted to write tests for it first.
This also changes [address:...] to not use a "0x" before the address, for consistency.
mstange
force-pushed
the
assembly-view-part-1
branch
from
4533a8b to
68c8d45
Compare
I missed these when addressing review comments in firefox-devtools#4478.
I missed these when addressing review comments in #4478.
2 participants
Broken out from #4196 for incremental review + landing.
I recommend reviewing this back-to-front: Start with the types
NativeSymbolInfo,BottomBoxInfo,SourceViewStateandAssemblyViewState. The rest of the patch just does the legwork to put the right values into those states.The
NativeSymbolInfois a thread-independent representation of a native symbol. Once the UI is implemented, you should be able double-click something in the call tree and then switch threads, and the assembly view should still display the right assembly code along with per-instruction timings which make sense for the selected thread. This would not be possible if we stored just a native symbol index, which would refer to different symbols in different threads.The assembly view is closed by default. Double-clicking a call node will now always update both the source view and the assembly view.
By itself, this patch doesn't change much. But it does have some observable effects on its own: