Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instruments importer #3684

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

gregtatum
Copy link
Member

@gregtatum gregtatum commented Nov 30, 2021

This was an attempt to port the speedscope importer to the profiler. I think it was sort of working when I stopped.

I initially attempted to fix up the intern's code, but ran into issues with not knowing what was a bug in the new code, or if there were bugs in the original code.

My process was to naively copy over the relevant speedscope code, and then re-work it to get the types passing. I think there were some existing issues with how an instruments profile was recorded and the final structure of recorded profile. There is an assumption in this code that the profile was either recorded from the command line, or recorded from the instruments app. I don't recall which at this point.

┆Issue is synchronized with this Jira Task

This commit should not be reviewed as code written by the author, but
rather as code that is imported pretty much directly. Only minimal
changes were made like removing export declarations, or import syntax.
This inlines the code from several different files as much as possible.
This code is useful if we want to support this feature, but for now,
just comment it out so that Flow doesn't complain.
This commit represents trivial lint and style fixes on the code. No
behavior was changed here.
This is the first real new code. It changes the implementation from
using the non-standard webkitEntries API, the JSZip API.
@jrmuizel
Copy link

I filed jlfwong/speedscope#384 for an Instruments file that wouldn't load with this and speedscope.

@gregtatum
Copy link
Member Author

In order to land this PR, the first few commits are all importing speedscope code and making it pass some basic CI requirements. These shouldn't be too controversial.

The two WIP commits on the end are the ones getting it to work for the Firefox Profiler. Honestly, they could probably be squashed into a single commit, and rebased, and landed as is.

I'm not planning on finishing this PR at this time.

There is an issue that @jrmuizel filed, which I believe that certain Instruments profiles have different file organization structure. I was testing this code from instruments-simple.trace.zip which appears to be working in the deploy preview.

@canova
Copy link
Member

canova commented Jun 27, 2022

It seems like they changed the Instruments file format quite a bit after Xcode 10.
Profiles from Xcode 10 load but with the wrong information. And profiles from Xcode 13 are not loading at all. It looks like there are a few problems that were identified in speedscope. Adding here so it could provide a basis for our investigation next time:

  1. Stack traces are messed up
  2. Thread IDs are incorrected (they don't match binary plist entries)
    This suggests that something changed either in bulkstore or in integeruniquer.data
    Attempted WIP fix: WIP: Fix import for some Instruments profiles jlfwong/speedscope#216
  3. Instead of being stored in the plist file the symbols are in the symbols directory. The symbols files are binary with a bunch of null terminated strings at the end for each of the symbols.

Interestingly the old Instruments profiles don't load in the new Instruments version as well. So it seems like they dropped the support for those profile types completely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants