Support the chrome JSON trace format (allows viewing of hermes traces)

zacharyfmarion commented 10 months ago

Add support for the chrome JSON object trace format. I have tested this with hermes profiles, which spit out a version of the spec with additional information in the stack frame. Mainly inspiration was taken from two sources:

Example profiles

Specifically I pulled some code from this file. While this works great for raw profiles, the hermes-profile-transformer package also applies sourcemaps, which is something that I would like to by applied for hermes profiles as well. It's a little awkward because in the hermes profile file format, there isn't actually a field from the file path where a given function in a frame originated - instead it is in parentheses after the function name. Also I will need to update the implementations in three places:

Which is a little overwhelming. Given time constraints I might not get a chance to land all of this for a while. I think in the shorter term it would be nice to just add some specific support for hermes-generated trace profiles in the trace-event file. Discussion on this is here

Example

Trace event format	Trace event object format

coveralls commented 10 months ago

coverage: 43.427% (+0.4%) from 43.041% when pulling c8c1e7f1af36f5fac5d62ee268ac62c56e3eda8b on zacharyfmarion:zac/hermes-format into dfd3a0dfb3b35d73b75023a1447604ebd721af12 on jlfwong:main.

zacharyfmarion commented 10 months ago

Actually after looking at this some more it looks like spec is actually part of the chrome trace event spec? But it doesn't currently seem to be implemented...at least hermes profiles did not work before. @jlfwong should I name this something more generic?

jlfwong commented 10 months ago

Going to continue discussion of both https://github.com/jlfwong/speedscope/pull/451#issuecomment-1865423405 and this PR here, since they're intertwined.

Given that this is an extension of the chrome trace event spec, this should IMO live inside of import/trace-event.ts. That file already contains handling of both a TraceEvent[] format and a {traceEvents: TraceEvent[]} format, so this seems like a fairly natural extension. And that unfortunately means it similarly should be generic w.r.t. handling of args.

That said, if there are highly reliable ways to see from analysis of a profile that it was specifically generated by Hermes, whether it be a {traceEvents, stackFrames, samples} profile as in this PR, or one of the existing supported trace event formats, I'm open to accepting a patch to trace-event.ts which ignores args in that case because they're known to be useless when coming from Hermes.

This could be through structural analysis of the JSON, or it could be from filename matching if Hermes spits out files with a specific naming convention.

Does that sound tenable?

zacharyfmarion commented 10 months ago

@jlfwong Okay sounds good - I will refactor trace-event to support the json object format - are you okay with me keeping the main json import code in a separate file? The trace-event file is already pretty large. Not sure if you're opposed to me creating a folder in the imports directory called traceEvent that has a types file and then two files, one for each import type. I can put it all in one file if you prefer.

Re. the hermes format - I should be able to reliably detect if the args match what hermes exports - thanks for understanding. I will do that after this lands and we can iterate. I definitely don't want to merge anything you're not comfortable with.

zacharyfmarion commented 10 months ago

Went ahead and pushed it up, can clean up a bit later

jlfwong commented 9 months ago

@zacharyfmarion thanks for being so responsive to feedback! is this ready to review now, or should I hold off for further changes?