Support inline callstack symbolication for local Firefox builds #3727

mstange · 2021-12-07T05:49:12Z

This PR makes it so that we will get inline callstacks when profiling local Firefox builds. It hooks up symbolication to the new querySymbolicationApi entry point from the WebChannel.

Example profile: https://share.firefox.dev/31CAqLf

Fixes #1516.

codecov · 2021-12-07T05:55:00Z

Codecov Report

Merging #3727 (586e8d9) into main (1e45932) will increase coverage by 0.01%.
The diff coverage is 93.44%.

❗ Current head 586e8d9 differs from pull request most recent head 2fd7f3b. Consider uploading reports for the commit 2fd7f3b to get more accurate results

@@            Coverage Diff             @@
##             main    #3727      +/-   ##
==========================================
+ Coverage   88.70%   88.72%   +0.01%     
==========================================
  Files         263      263              
  Lines       22100    22129      +29     
  Branches     5660     5666       +6     
==========================================
+ Hits        19604    19634      +30     
+ Misses       2313     2312       -1     
  Partials      183      183

Impacted Files	Coverage Δ
src/profile-logic/mozilla-symbolication-api.js	`13.25% <40.00%> (-1.04%)`	⬇️
src/profile-logic/symbol-store.js	`93.75% <97.05%> (-0.07%)`	⬇️
src/actions/receive-profile.js	`86.48% <100.00%> (+0.27%)`	⬆️
src/app-logic/browser-connection.js	`86.66% <100.00%> (+1.30%)`	⬆️
src/components/timeline/FullTimeline.js	`67.56% <0.00%> (ø)`
src/app-logic/web-channel.js	`96.66% <0.00%> (+3.33%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e45932...2fd7f3b. Read the comment docs.

julienw

Thanks, let's move forward with that!
That's a pretty big change in our symbolication code, and this all works fine!

julienw · 2021-12-13T14:56:33Z

src/profile-logic/mozilla-symbolication-api.js

-
-  const responseJson = _ensureIsAPIResultV5(await response.json());
+  const responseJson = _ensureIsAPIResultV5(
+    await callback('/symbolicate/v5', JSON.stringify(body))


I wonder if we should pass body and let the callback call JSON.stringify if necessary only.

That would be an option. I don't have an opinion on it. Both callees currently need JSON.stringify.

julienw · 2021-12-13T16:46:18Z

src/app-logic/browser-connection.js

+      );
+    }
+
+    return querySymbolicationApiViaWebChannel(path, requestJson);


I'm wondering what Firefox does with the path later on? I think it passes it along all the way to the wasm program, but I wonder why it's useful?
(probably not for this PR)

It's useful so that we can add more APIs without having to add more methods to the WebChannel. I want to add two new APIs: A way to get source code, and a way to get assembly code.
I've already landed a /source/v1 API in profiler-get-symbols (but not integrated it into Firefox yet): mstange/profiler-get-symbols#24

julienw · 2021-12-13T16:53:39Z

src/profile-logic/symbol-store.js

+          // No symbols for this library, it'll need to try the next option.
+          const { request, error } = response;
+          failedRequests.push(request);
+          (errorMap.get(request) ?? []).push(error);


Is that working? If the request isn't in errorMap yet, how does it get there?
(same remark for the same line below)

Ah, maybe I should add a comment. request is guaranteed to be in the errorMap - we initialize errorMap with all requests. The ?? [] is just there because it's the shortest way I could think of to appease Flow. In Rust I would use .unwrap() here to say "I'm sure that this exists and I'm willing to risk a panic if my reasoning about this code is wrong". In JS I could throw an exception, but that would be three extra lines.
Or maybe I should use ensureExists from the flow helpers? But that one takes an error message...

I've added two comments.

Or maybe I should use ensureExists from the flow helpers? But that one takes an error message...

The error message isn't mandatory, the tool adds a default one if it's missing.

So that would have been my preference :-)

julienw · 2021-12-13T17:00:08Z

src/profile-logic/symbol-store.js

+          // Some libraries are still remaining, try option 3 for them.
+          // Option 3 is symbolProvider.requestSymbolsFromBrowser.
+          const option3ResponsePromise =
+            this._symbolProvider.requestSymbolsFromBrowser(option3Requests);


The name of the functions requestSymbolsFromBrowser vs requestSymbolTableFromBrowser are very close, do you have an idea to make them more descriptive? Maybe requestSymbolsFromWebChannel vs requestSymbolTableFromFrameScript? But for Android we're not using the framescript, are we?

Yes, the names are quite close indeed. I'm hoping we can remove the "symbol table" variants once we've converted Android (i.e. fixed bug 1735897).

As for the WebChannel / frame script distinction vs requestSymbols / requestSymbolTable, the relationship is as follows:

SymbolStore --> requestSymbolsFromBrowser --> WebChannel querySymbolicationApi (if available) | `-------> requestSymbolTableFromBrowser --> WebChannel getSymbolTable (if available) | `-----> otherwise frame script getSymbolTable

julienw · 2021-12-13T17:05:39Z

src/profile-logic/mozilla-symbolication-api.js

  requests: LibSymbolicationRequest[],
-  symbolsUrl: string
+  callback: (path: string, requestJson: string) => Promise<MixedObject>


I believe the name callback isn't descriptive enough, what do you think of renaming it symbolFetcherCallback or something similar?
Also please describe it a bit in the comment above.

Good suggestions, will do.

I've renamed this to querySymbolicationApiCallback and also created a type QuerySymbolicationApiCallback for it.

The errors created here are later accumulated into one big SymbolsNotFoundError, once all options have failed. At that point we also still know the lib. There is no need to embed the lib into the error here, too. We have an instanceof SymbolsNotFoundError check, but that check only ever sees the combined error. It doesn't look at the individual input errors. So this doesn't change any behavior.

It now receives a callback so that it supports both doing a fetch request to a server and sending the request elsewhere, for example to the browser.

This calls through into BrowserConnection.querySymbolicationApi. The request and response JSON are processed by MozillaSymbolicationAPI.

We insert this as the new option 3, before getting symbol tables (which is now option 4). This gives us rich symbol information with file + line + inline data. We keep option 4 for old Firefox versions and for Android system libraries.

mstange requested a review from julienw December 7, 2021 05:49

mstange self-assigned this Dec 7, 2021

mstange mentioned this pull request Dec 7, 2021

[Deploy preview] Inline callstacks demo #2556

Closed

mstange force-pushed the query-symbolication-api branch from 7694e60 to 586e8d9 Compare December 13, 2021 14:35

julienw approved these changes Dec 13, 2021

View reviewed changes

mstange added 6 commits December 14, 2021 13:07

Small clean-up.

39761f9

Add BrowserConnection.querySymbolicationApi.

2769790

Make mozilla-symbolication-api.js more generic.

c0b9f36

It now receives a callback so that it supports both doing a fetch request to a server and sending the request elsewhere, for example to the browser.

Add SymbolStore.requestSymbolsFromBrowser.

e90be17

This calls through into BrowserConnection.querySymbolicationApi. The request and response JSON are processed by MozillaSymbolicationAPI.

Use SymbolStore.requestSymbolsFromBrowser.

2fd7f3b

We insert this as the new option 3, before getting symbol tables (which is now option 4). This gives us rich symbol information with file + line + inline data. We keep option 4 for old Firefox versions and for Android system libraries.

mstange force-pushed the query-symbolication-api branch from 586e8d9 to 2fd7f3b Compare December 14, 2021 12:19

mstange enabled auto-merge December 14, 2021 12:21

mstange merged commit 46ddaf8 into firefox-devtools:main Dec 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inline callstack symbolication for local Firefox builds #3727

Support inline callstack symbolication for local Firefox builds #3727

mstange commented Dec 7, 2021 •

edited

codecov bot commented Dec 7, 2021 •

edited

julienw left a comment

julienw Dec 13, 2021

mstange Dec 13, 2021

julienw Dec 13, 2021

mstange Dec 13, 2021

julienw Dec 13, 2021

mstange Dec 13, 2021

mstange Dec 14, 2021

julienw Dec 14, 2021

julienw Dec 14, 2021

julienw Dec 13, 2021

mstange Dec 13, 2021

julienw Dec 13, 2021

mstange Dec 13, 2021

mstange Dec 14, 2021

Support inline callstack symbolication for local Firefox builds #3727

Support inline callstack symbolication for local Firefox builds #3727

Conversation

mstange commented Dec 7, 2021 • edited

codecov bot commented Dec 7, 2021 • edited

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstange commented Dec 7, 2021 •

edited

codecov bot commented Dec 7, 2021 •

edited