Assembly view by mstange · Pull Request #4196 · firefox-devtools/profiler

mstange · 2022-08-19T14:37:22Z

This adds an assembly view to the bottom box.

It currently works with Firefox 110+ when looking at a freshly-captured profile, but not when looking at an existing uploaded profile. To test:

Set your devtools.performance.recording.ui-base-url pref to https://deploy-preview-4196--perf-html.netlify.app/.
Capture a profile.
Double-click a C++ function, once symbolication has completed.
Click the "asm" icon next to the close button of the bottom box.

It also works with freshly-captured profiles from samply. To test:

Run PROFILER_URL='https://deploy-preview-4196--perf-html.netlify.app/' samply record [...]
Same as above.

Deploy preview

codecov · 2023-02-15T17:34:12Z

Codecov Report

Patch coverage: 77.07% and project coverage change: +0.14 🎉

Comparison is base (d550813) 88.48% compared to head (07f1300) 88.62%.

❗ Current head 07f1300 differs from pull request most recent head d675127. Consider uploading reports for the commit d675127 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4196      +/-   ##
==========================================
+ Coverage   88.48%   88.62%   +0.14%     
==========================================
  Files         287      293       +6     
  Lines       25807    25980     +173     
  Branches     6946     6998      +52     
==========================================
+ Hits        22836    23026     +190     
+ Misses       2765     2748      -17     
  Partials      206      206

Impacted Files	Coverage Δ
src/components/app/CodeErrorOverlay.js	`0.00% <0.00%> (ø)`
src/components/shared/SourceView-codemirror.js	`80.48% <ø> (-10.43%)`	⬇️
src/components/app/CodeLoadingOverlay.js	`58.33% <58.33%> (ø)`
src/components/app/BottomBox.js	`74.50% <70.27%> (+35.62%)`	⬆️
src/components/shared/SourceView.js	`83.63% <75.00%> (-1.85%)`	⬇️
src/components/shared/AssemblyView-codemirror.js	`84.33% <84.33%> (ø)`
src/components/shared/AssemblyView.js	`87.71% <87.71%> (ø)`
src/utils/codemirror-shared.js	`98.27% <98.27%> (ø)`
src/actions/code.js	`50.00% <100.00%> (+25.00%)`	⬆️
src/components/app/AssemblyViewToggleButton.js	`100.00% <100.00%> (ø)`
... and 2 more

... and 6 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mstange · 2023-03-09T23:48:21Z

This is now ready for review!

canova

Looks great to me! Thanks for the work, it's really exciting to have the assembly view!

canova · 2023-03-14T14:45:20Z

+.CodeLoadingOverlay,
+.SourceCodeErrorOverlay {


nit: Are the uppercase first letters on these class names on this file intentional or artifact of some find/replace? If I'm not mistaken, we usually use camelCase for class names. I would prefer to keep using the same styling if it wasn't intentional.

Ah, they were somewhat intentional because I thought I remembered seeing this form elsewhere, but I must have imagined it. I'll change it to camelCase for consistency. Though we do seem to be a bit unsure about whether to use camelCase or kebab-case, I can find recent examples for both.

Yeah, we should fix those inconsistencies as well. I mostly see kebab-case on class names that are related to photon like photon-button. Maybe they were imported directly from photon?
But there are also class names that are not tied to photon too.

My (non explicit) styleguide was that I used kebab-case for project-transverse styles (such as photon but not only), and camelCase (like it was before) for component-specific styles. TBH I would prefer with the uppercase first letters for these ones because then it's more obvious they're tied to a component.

canova · 2023-03-14T15:10:06Z

+  background-color: var(--grey-30) !important;
+}
+
+/* Provide 3px extra grabbable surface on each side of the splitter */


When you start dragging that extra 3px grabbable area, the splitter jumps because of it. But probably that's not a big issue.

I noticed that too and filed zesik/react-splitter-layout#67 about it, but the repo seems somewhat unmaintained. It also happens with a regular wide splitter, it's not because of the negative margins, from what I can tell.

I see, thanks for filing! Yeah, last commit is from 2019. Doesn't look like it's maintained

canova · 2023-03-14T17:04:20Z


+function convertErrors(errors: ApiQueryError[]): SourceCodeLoadingError[] {
+  // Copy the array so that the types work out.
+  return errors.map((e) => e);


Hmm this looks a bit unnecessary. I wonder if we can let flow understand this without copying. I wanted to check it locally but looks like I need to fix my flow setup first :')

I think it's necessary unless there's a way to tell Flow that we will not mutate the result.

If we don't copy the array here, I can see why Flow doesn't like it.

const apiErrors = [{ type: 'BROWSER_API_ERROR', apiErrorMessage: '' }]; const sourceCodeLoadingErrors = convertErrors(apiErrors); sourceCodeLoadingErrors.push({ type: 'NO_KNOWN_CORS_URL' }); // Now apiErrors has an object in it which is not allowed by the ApiQueryError type!

I see, thanks for the explanation. That works for me then.

@mstange This seems to work for me with a $ReadOnlyArray:

diff --git a/src/components/app/BottomBox.js b/src/components/app/BottomBox.js index 348ecd048..27b882283 100644 --- a/src/components/app/BottomBox.js +++ b/src/components/app/BottomBox.js @@ -34,18 +34,16 @@ import explicitConnect from 'firefox-profiler/utils/connect'; import type { ConnectedProps } from 'firefox-profiler/utils/connect'; import type { LineTimings, AddressTimings, SourceCodeStatus, AssemblyCodeStatus, NativeSymbolInfo, - SourceCodeLoadingError, - ApiQueryError, } from 'firefox-profiler/types'; import type { CodeErrorOverlayProps } from './CodeErrorOverlay'; import { Localized } from '@fluent/react'; import './BottomBox.css'; type StateProps = {| @@ -257,34 +255,27 @@ class BottomBoxImpl extends React.PureComponent<Props> { /> ) : null} {assemblyViewCode !== undefined && assemblyViewCode.type === 'LOADING' ? ( <CodeLoadingOverlay source={assemblyViewCode.source} /> ) : null} {assemblyViewCode !== undefined && assemblyViewCode.type === 'ERROR' ? ( - <AssemblyCodeErrorOverlay - errors={convertErrors(assemblyViewCode.errors)} - /> + <AssemblyCodeErrorOverlay errors={assemblyViewCode.errors} /> ) : null} </div> </div> ) : null} </SplitterLayout> </div> ); } } -function convertErrors(errors: ApiQueryError[]): SourceCodeLoadingError[] { - // Copy the array so that the types work out. - return errors.map((e) => e); -} - export const BottomBox = explicitConnect<{||}, StateProps, DispatchProps>({ mapStateToProps: (state) => ({ sourceViewFile: getSourceViewFile(state), sourceViewCode: getSourceViewCode(state), globalLineTimings: selectedThreadSelectors.getSourceViewLineTimings(state), selectedCallNodeLineTimings: selectedNodeSelectors.getSourceViewLineTimings(state), sourceViewScrollGeneration: getSourceViewScrollGeneration(state), diff --git a/src/components/app/CodeErrorOverlay.js b/src/components/app/CodeErrorOverlay.js index f833fd19c..f647c1db9 100644 --- a/src/components/app/CodeErrorOverlay.js +++ b/src/components/app/CodeErrorOverlay.js @@ -5,17 +5,17 @@ import React from 'react'; import { assertExhaustiveCheck } from 'firefox-profiler/utils/flow'; import type { SourceCodeLoadingError } from 'firefox-profiler/types'; import { Localized } from '@fluent/react'; export type CodeErrorOverlayProps = {| - errors: SourceCodeLoadingError[], + errors: $ReadOnlyArray<SourceCodeLoadingError>, |}; export function CodeErrorOverlay({ errors }: CodeErrorOverlayProps) { return ( <ul className="codeErrorOverlay"> {errors.map((error, key) => { switch (error.type) { case 'NO_KNOWN_CORS_URL': {

Am I missing something?

Nice! I wasn't aware of $ReadOnlyArray. I had briefly tried $ReadOnly<SourceCodeLoadingError[]> but I've never really worked with these special types and didn't check the documentation.

Do you want to submit this patch as a PR or would you like me to do it?

I cleaned up my local copy already oops :) I'm happy if you can do it and double check that I didn't forget anything, and I can review!

This also updates some L10nIDs and some strings. In particular, it changes the string "Source not available" to "Source code not available", which is less ambiguous.

mstange force-pushed the assembly-view branch from 73a6e63 to a43e133 Compare November 29, 2022 18:22

mstange force-pushed the assembly-view branch from a43e133 to 8890fa6 Compare December 16, 2022 19:54

mstange force-pushed the assembly-view branch from 8890fa6 to ceef35a Compare January 12, 2023 21:09

mstange mentioned this pull request Jan 14, 2023

There should be a context menu item for opening the source view #4405

Closed

mstange force-pushed the assembly-view branch 3 times, most recently from 7109700 to e4409d8 Compare February 10, 2023 20:36

mstange force-pushed the assembly-view branch 3 times, most recently from 292be1e to f76542a Compare February 15, 2023 17:27

mstange force-pushed the assembly-view branch 3 times, most recently from c50b601 to dd7eaa2 Compare February 16, 2023 21:12

mstange mentioned this pull request Feb 17, 2023

Assembly view part 1 #4478

Merged

mstange force-pushed the assembly-view branch from dd7eaa2 to b334336 Compare February 17, 2023 06:03

mstange force-pushed the assembly-view branch from b334336 to 6d75828 Compare February 24, 2023 18:56

mstange force-pushed the assembly-view branch 6 times, most recently from 57ee618 to 63e0ef5 Compare March 9, 2023 23:47

mstange marked this pull request as ready for review March 9, 2023 23:47

mstange requested a review from a team as a code owner March 9, 2023 23:47

mstange changed the title ~~[WIP] Assembly view~~ Assembly view Mar 9, 2023

mstange requested a review from canova March 9, 2023 23:48

mstange self-assigned this Mar 9, 2023

flodolo reviewed Mar 11, 2023

View reviewed changes

Comment thread locales/en-US/app.ftl Outdated

mstange force-pushed the assembly-view branch from 63e0ef5 to 07f1300 Compare March 13, 2023 23:20

flodolo approved these changes Mar 14, 2023

View reviewed changes

Comment thread locales/en-US/app.ftl Outdated

Comment thread locales/en-US/app.ftl Outdated

Comment thread locales/en-US/app.ftl

canova approved these changes Mar 14, 2023

View reviewed changes

mstange added 2 commits March 14, 2023 14:43

Move mapGetKeyWithMaxValue into src/utils/.

aaf5924

Rename source to code or sourceCode in more places.

f9ad9da

mstange force-pushed the assembly-view branch from 07f1300 to c723484 Compare March 14, 2023 19:10

mstange added 3 commits March 14, 2023 15:12

Move some CodeMirror code.

450f89c

Move loading and error UI code.

18d5ccc

This also updates some L10nIDs and some strings. In particular, it changes the string "Source not available" to "Source code not available", which is less ambiguous.

Remove unused SourceView CSS.

9647b43

mstange force-pushed the assembly-view branch from c723484 to 2510a7f Compare March 14, 2023 19:13

mstange added 3 commits March 14, 2023 15:14

Make SourceView CSS shareable.

d929257

Add an AssemblyViewToggleButton component.

cdd2812

Add AssemblyView UI.

6c8cac8

mstange force-pushed the assembly-view branch from 2510a7f to 2bf24c3 Compare March 14, 2023 19:17

Add a test for the assembly view.

d675127

mstange force-pushed the assembly-view branch from 2bf24c3 to d675127 Compare March 14, 2023 19:32

mstange enabled auto-merge March 14, 2023 19:33

mstange merged commit 5bc6261 into firefox-devtools:main Mar 14, 2023

		.CodeLoadingOverlay,
		.SourceCodeErrorOverlay {

Conversation

mstange commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mstange commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mstange commented Aug 19, 2022 •

edited

Loading

codecov Bot commented Feb 15, 2023 •

edited

Loading