Support importing Instruments profiles by rajmeghpara · Pull Request #2138 · firefox-devtools/profiler

rajmeghpara · 2019-07-09T16:58:44Z

As a part of GSoC’19, this PR makes Firefox Profiler able to process and visualize profiles generated by Instruments Profiler which is a great macOS sampling profiler tool that’s part of Xcode toolset.

This allows users on macOS to use Firefox Profiler for analyzing and visualizing the Instruments profiles.

Example Instruments profile in Firefox Profiler: https://perfht.ml/2OOYDGY

Call Tree:

Flame Graph:

Stack Chart:

┆Issue is synchronized with this Jira Task

canova

Just skimmed the code a bit, so this is not a review but wanted to post the things I saw as an early feedback.
Overall, I think we could use lots of comments, especially comments on top of functions that explains the behavior of it.

codecov · 2019-08-04T13:32:27Z

Codecov Report

❌ Patch coverage is 84.15033% with 97 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@8aa9e8e). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/profile-logic/import/instruments/index.js	87.46%	45 Missing and 6 partials ⚠️
...file-logic/import/instruments/BinaryPlistParser.js	87.20%	15 Missing and 1 partial ⚠️
src/components/app/Home.js	0.00%	10 Missing and 4 partials ⚠️
src/profile-logic/import/instruments/BinReader.js	67.74%	9 Missing and 1 partial ⚠️
...-logic/import/instruments/MaybeCompressedReader.js	83.87%	5 Missing ⚠️
src/actions/receive-profile.js	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2138   +/-   ##
=======================================
  Coverage        ?   85.77%           
=======================================
  Files           ?      204           
  Lines           ?    14602           
  Branches        ?     3658           
=======================================
  Hits            ?    12525           
  Misses          ?     1899           
  Partials        ?      178

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…e-profile.js

- Implement readFormTemplateFile function to get and read the form.template file - Implement readInstrumentsArchive (helper function for readFormTemplateFile) - Implement parseBinaryPlist (helper function for readInstrumentsArchive) - Implement BinaryPlistParser class to parse the binary plist data

…er functions

- This function extracts the samples (timestamp, threadID, backtraceID)

This map helps resolving backtraceID field inside each sample tuple

- Add check for not processing same run again

- Extract threadIDToSamples map <threadId, [samples]>

- Fill FuncTable - Fill FrameTable - Fill StackTable - Fill SamplesTable

Update the timestamp

julienw · 2019-11-18T14:50:04Z

Thanks @rajmeghpara, is it ready for another review?

rajmeghpara · 2019-11-18T19:09:34Z

@julienw , There are 8 more comments which are unresolved. I'll try to resolve all of them by this week. Thanks! :)

julienw · 2019-11-19T08:27:51Z

Thanks for the update!

rajmeghpara · 2019-11-23T10:46:36Z

@julienw, Apple BinaryPlist data format is very unstructured.
For example, root.$objects (one of the parameter in expandKeyedArchive function) array looks like this:

So it's hard to define a type for this kind of arrays or objects. That's why I had to put any types for these kinds of instances.

cc: @canova

julienw · 2019-12-07T16:40:45Z

hey raj, a quick note that we didn't forget you, but we've been super busy lately. I still want to look at this soon...

rajmeghpara · 2019-12-07T22:21:28Z

@julienw, It's completely fine :)
Please review when you get time.

julienw

Hey @raj, I'm deeply sorry for the delay to look at this again.

I'm a bit confused because I see that a lot of my comments have been marked as "resolved" but they're not been handled. Maybe you forgot to commit something?
I unresolved the comments where I don't see the fix, and added a bit more detailed code bits for some parts. I think that most of the comments are pretty straightforward, except maybe the tests. Tell me what you think!
Also again please tell me if you'd like that we take it from there. This is a quite big piece of work and we'd understand if you're busy with other thing!
Thanks again for your work!

julienw · 2019-12-12T17:53:18Z

+
+    let webkitEntry = null;
+
+    if ('webkitGetAsEntry' in firstItem) {


If items has a length of 0, firstItem will be undefined, and this if condition will throw an error.
Also items isn't supported in all browsers yet, only chromium- and gecko-based browsers, and in that case the destructuring above will also fail.
Therefore I think it's better to be a bit more defensive here:

const { files, items, } = event.dataTransfer; if (items && items.length > 0) { ... } if (files.length > 0) { ... }

I know you already rewrote this part several times, but I think this is important. Sorry for going back and forth on this.

julienw · 2019-12-13T11:12:40Z

+// This function constructs an object from given interpreter class and property list
+// I have taken inspiration for this function from here:
+// https://github.com/jlfwong/speedscope/blob/9edd5ce7ed6aaf9290d57e85f125c648a3b66d1f/import/instruments.ts#L648
+function patternMatchObjectiveC(


can you please write a comment explaining why we have so many any types here?

I have mentioned the reason for this comment here: #2138 (comment)
I will also add it to the file.

julienw · 2019-12-13T11:13:44Z

+// This function adds a padding ahead of given string to make its length equal to a given width value
+function zeroPad(s: string, width: number) {
+  return new Array(Math.max(width - s.length, 0) + 1).join('0') + s;
+}
+


Please just use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/padStart

julienw · 2019-12-13T12:53:51Z

+          errCb(err);
+        }
+      )
+      .catch(errCb);


What I mean is that maybe we want :

.then( blob => { ... cb(blob) }, errCb );

The difference is that in your code errors thrown by cb are caught by errCb, but in the code I've put above, errCb will be called only if the zip operation fails.
The difference is tiny but I think this is more correct.

julienw · 2019-12-13T13:01:53Z

+describe('convertInstrumentsProfile function', () => {
+  async function importFromTrace(tracePath: string, fileName: string) {
+    const zip = await new Promise<any>((resolve, reject) => {
+      return fs.readFile(tracePath, (err, data) => {


This is what I had in mind:

const data = fs.readFileSync(tracePath); const zip = await JSZip.loadAsync(data);

I think this is less verbose and clearer :-)

julienw · 2019-12-13T13:44:11Z

+declare module 'pako' {
+  declare module.exports: any;
+}


Can we please type at least the inflate function?
I think this works:

declare module 'pako' { declare type pako$InflateOptions = {| +windowBits?: number, +raw?: boolean, +chunkSize?: number, +dictionary?: string | ArrayBuffer | Uint8Array, |}; declare function inflate(buffer: Uint8Array | number[] | string, options?: pako$InflateOptions): Uint8Array; declare function inflate(buffer: Uint8Array | number[] | string, options: {| ...pako$InflateOptions, +to: 'string' |}): string; declare module.exports: { inflate: typeof inflate }; }

rajmeghpara · 2019-12-14T12:15:21Z

@julienw,
I have resolved your latest comments and made the changes.
Thanks for your valuable comments!

mstange · 2020-05-03T18:15:38Z

Whoa, I just found some actual documentation! It's packaged inside Instruments.app:
file:///Applications/Xcode.app/Contents/Applications/Instruments.app/Contents/Packages/Sampling.instrdst/Contents/Documentation/fulldoc.html

If you view a profile in Instruments, you can access this documentation by going to: Main Menu -> Instrument -> Instrument Inspector -> Schemas -> time-profile.
This documentation links to documentation on help.apple.com such as the one for the "backtrace" engineering type: https://help.apple.com/instruments/developer/mac/current/#/dev15401019

mstange · 2020-05-03T18:25:56Z

I have a bug report: Inlined frames seem to be assigned to the wrong parent. It seems they're lifted up one level and become a sibling of their actual parent.
To reproduce, load this trace: https://drive.google.com/file/d/1fAMdpODaax4Nk4m5jLUbOdulLt_y8MG2/view?usp=sharing

Instruments (correct):

Firefox profiler (incorrect): https://perfht.ml/3aY8qAx

In this example, the "count" method (_$LT$core..str..Chars$u20$as$u20$core..iter..traits..iterator..Iterator$GT$::count::h44f4fc8cdfe212bc) should be called by the "levenshtein_distance" method (rust::levenshtein_distance::levenshtein_distance::h9950dc9c996816c3). But in the imported profile, the "count" method is called by the "main" method instead (rust::main::_$u7b$$u7b$closure$u7d$$u7d$::h880a062807c5a68a).

mstange · 2020-05-03T18:37:57Z

+  const backtraceIDtoStack = new Map<number, Array<number>>();
+
+  function appendRecursive(k: number, stack: Array<number>) {
+    const frame = addressToFrameMap.get(k);
+    if (frame) {
+      stack.push(k);
+      return;
+    }
+
+    const arrayIndex = k & 0xffffffff;
+    if (arrayIndex in arrays) {
+      for (const addr of arrays[arrayIndex]) {
+        appendRecursive(addr, stack);
+      }
+      return;
+    }


Could we get some better names here, and maybe a comment on how this structure works?
So the result is a "backtraceIDToStack" map. In the lambda below, the keys in this map are called "id". And "id" is passed to the "k" argument of appendRecursive. And then k is used as a key to do a lookup in addressToFrameMap. So is k a backtraceID or an address?

And then appendRecursive is both recursive and has a for-of loop in it. It seems like there's a tree structure underlying here, where each backtraceID refers to a list of other backtraceIDs (let's call them children), and appendRecursive traverses the subtree of a given backtraceID in a depth-first search. So the "stack" that's output is really a list of the nodes that were encountered, in traversal order, during this DFS. This is quite unexpected! Is there some intuitive explanation of this graph structure?

mstange · 2020-07-15T02:25:50Z

This implementation also seems to create huge frameTables / funcTables / stringTables. It seems every thread has all the strings for all the threads. And the frameTable.address values seem wrong.

julienw · 2025-07-11T15:23:44Z

Is it worth keeping this PR open?

canova · 2026-03-03T16:28:12Z

Let's close this one. It's been a while, and there was another attempt on instruments importer in #3684.

rajmeghpara force-pushed the instruments branch from 09ba5cc to 14e4715 Compare July 9, 2019 17:51

rajmeghpara commented Jul 9, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/instruments.js Outdated

canova reviewed Jul 10, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/BinaryPlistParser.js Outdated

canova reviewed Jul 10, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/instruments.js Outdated

mstange reviewed Jul 11, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/instruments.js Outdated

rajmeghpara force-pushed the instruments branch 3 times, most recently from d2aa38e to 99e0da5 Compare July 13, 2019 11:13

gregtatum reviewed Jul 29, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/index.js Outdated

gregtatum reviewed Jul 29, 2019

View reviewed changes

Comment thread src/profile-logic/import/instruments/MaybeCompressedReader.js Outdated

rajmeghpara force-pushed the instruments branch from 02874e9 to 9e05c2c Compare August 5, 2019 15:45

rajmeghpara changed the title ~~[wip] Support importing Instruments profiles~~ Support importing Instruments profiles Aug 10, 2019

rajmeghpara force-pushed the instruments branch 2 times, most recently from 3352143 to 09c4367 Compare August 11, 2019 16:56

rajmeghpara added 14 commits August 11, 2019 22:35

Add isInstrumentsProfile function and integration of it inside receiv…

23cf52a

…e-profile.js

Remove .idea/ from ./gitignore file

9afacb7

Extract files and directories in form of tree

5a0fd89

Doubt regarding output of parseBinaryPlist function

944eb96

Implemented expandKeyedArchive, patternMatchObjectiveC and other help…

7e7edc6

…er functions

Extract runs, instruments type, version and selectedRunNumber

ccbeff5

Implement importRunFromInstrumentsTrace function

a1d046a

- This function extracts the samples (timestamp, threadID, backtraceID)

Implement backtraceIDtoStack map

13bbc15

This map helps resolving backtraceID field inside each sample tuple

- Populate stackTrace inside each sample

6967c39

- Add check for not processing same run again

- Start implementing the pushThreadsInProfile function

bcac0ec

- Extract threadIDToSamples map <threadId, [samples]>

Implement fillThread function

aefe23a

- Fill FuncTable - Fill FrameTable - Fill StackTable - Fill SamplesTable

Update the fileName

c0fbb06

Update the timestamp

Process just first run and ignore others

6ff3c49

rajmeghpara added 2 commits November 17, 2019 18:40

- Resolve comments

f818b08

- Resolve comments

6b02380

rajmeghpara force-pushed the instruments branch from 8528195 to 6b02380 Compare November 17, 2019 13:59

rajmeghpara added 2 commits November 23, 2019 17:13

- Resolve comments

448ee92

- Resolve comments

ba962ea

julienw self-requested a review December 2, 2019 18:05

- Add description in index.js

8e5c217

julienw requested changes Dec 13, 2019

View reviewed changes

- Resolve comments

a144941

- Add comment

065af02

mstange reviewed May 3, 2020

View reviewed changes

julienw self-requested a review June 17, 2020 07:03

gregtatum closed this Jun 30, 2020

gregtatum reopened this Jun 30, 2020

gregtatum changed the base branch from master to main June 30, 2020 19:25

julienw removed their request for review July 11, 2025 15:23

mstange mentioned this pull request Sep 4, 2025

Remove the FlowFixMe once getAsEntry is a standard function and has a flow type #2230

Closed

canova closed this Mar 3, 2026


		let webkitEntry = null;

		if ('webkitGetAsEntry' in firstItem) {

Uh oh!

Conversation

rajmeghpara commented Jul 9, 2019 • edited by data-sync-user Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw commented Nov 18, 2019

Uh oh!

rajmeghpara commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julienw commented Nov 19, 2019

Uh oh!

rajmeghpara commented Nov 23, 2019

Uh oh!

julienw commented Dec 7, 2019

Uh oh!

rajmeghpara commented Dec 7, 2019

Uh oh!

julienw left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajmeghpara commented Dec 14, 2019

Uh oh!

mstange commented May 3, 2020

Uh oh!

mstange commented May 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstange commented Jul 15, 2020

Uh oh!

julienw commented Jul 11, 2025

Uh oh!

canova commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rajmeghpara commented Jul 9, 2019 •

edited by data-sync-user

Loading

codecov Bot commented Aug 4, 2019 •

edited

Loading

rajmeghpara commented Nov 18, 2019 •

edited

Loading