Remove the makeProfileSerializable step - make the raw in-memory profile match the format that's stored in the file#5287
Conversation
2ef3128 to
cf6429d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5287 +/- ##
==========================================
- Coverage 86.07% 86.03% -0.04%
==========================================
Files 311 311
Lines 29657 29805 +148
Branches 8196 8217 +21
==========================================
+ Hits 25528 25644 +116
- Misses 3547 3573 +26
- Partials 582 588 +6 ☔ View full report in Codecov by Sentry. |
cf6429d to
69e38e1
Compare
50bcd43 to
9f2dbb7
Compare
cfdcc2e to
e237bba
Compare
Two small changes in preparation for #5287. No changes in behavior.
e237bba to
f385591
Compare
6b7b61d to
63aa47e
Compare
Three more preparations for #5287, no changes in behavior.
3a328ac to
b9bd012
Compare
12e8390 to
64c701c
Compare
julienw
left a comment
There was a problem hiding this comment.
I haven't fully finished yet but I need to switch my computer, so pushing what I've got so far :)
| // doesn't have the string "IPC". This lets us avoid looping over all the | ||
| // markers when we don't have to. | ||
| if (!thread.stringTable.hasString('IPC')) { | ||
| const stringTable = StringTable.withBackingArray(thread.stringArray); |
There was a problem hiding this comment.
could be worth creating outside of the loop, even if it's just a WeakMap lookup
There was a problem hiding this comment.
Outside the loop over the threads? But it's per thread.
There was a problem hiding this comment.
mmm right, I was confused here
| function getScriptLocationToFuncIndex( | ||
| thread: RawThread | ||
| thread: RawThread, | ||
| stringTable: StringTable |
There was a problem hiding this comment.
I was wondering why you decided to pass it instead of rebuild it here (given it's not a hot loop) for simplicity, but I don't really mind because we're in the js-tracer code.
There was a problem hiding this comment.
Not sure either, but I think it'll change anyway once we start sharing the string table between threads. I'll leave it as-is.
| } | ||
| const profile = profiles[i]; | ||
| let thread = { ...profile.threads[selectedThreadIndex] }; | ||
| thread.stringArray = thread.stringArray.slice(); |
There was a problem hiding this comment.
I may be wrong but I think it's not necessary to slice it, it's never modified.
(BTW even if it was modified I think it's not even a big deal in the "diffing" case because the downloaded profiles are not used later.. but for correctness it would have been better)
There was a problem hiding this comment.
true, I'll remove the line
| const { categories, shutdownTime } = meta; | ||
|
|
||
| const stringTable = StringTable.withBackingArray(thread.stringTable); | ||
| const mutatedStringArray = thread.stringTable.slice(); |
There was a problem hiding this comment.
Is it necessary to slice it before mutating, given we'll throw away the gecko thread data afterwards?
(not opposed if you prefer to keep it this way)
There was a problem hiding this comment.
I agree with what you're saying. The existing code appears to treat the gecko profile as immutable, so I did the same, but I agree that we throw the Gecko profile out afterwards. I will remove the slice call.
| const getStringArray: Selector<string[]> = (state) => | ||
| getRawThread(state).stringArray; |
There was a problem hiding this comment.
(optional)
Is creating a selector function for that useful? is it used elsewhere?
Otherwise I'd just use this input selector inline in getStringTable.
There was a problem hiding this comment.
You're right, I'll just inline it.
| > | ||
| ( | ||
| 1.65 kB | ||
| 1.64 kB |
There was a problem hiding this comment.
Yeah I'd love to know what that's about! :D
It was a source of many rebase conflicts.
| const screenshotNameIndex = stringTable.indexForString( | ||
| 'CompositorScreenshot' | ||
| ); | ||
| const screenshotNameIndex = stringArray.indexOf('CompositorScreenshot'); |
There was a problem hiding this comment.
is it worth getting the string table from the cache with withBackingArray?
There was a problem hiding this comment.
I think what I had in mind here is that computeActiveTabTracks is called very early and it iterates over every thread, and I wanted to avoid creating the stringTable's map for threads / tracks that we don't display. But I think something else would have created it anyway. And in the future we'll only have one stringTable anyway so it's not worth optimizing this case. I'll change it as you suggested.
| | 'teal' | ||
| | 'yellow'; | ||
|
|
||
| export type RawCounter = {| |
There was a problem hiding this comment.
The type looks identical to Counter except for the type for samples, is it worth recreating it fully, or could we use generics for it? For example:
export type GenericCounter<SamplesTable> = {
...
samples: SamplesTable,
}
export type Counter = GenericCounter<CounterSamplesTable>;
export type RawCounter = GenericCounter<RawCounterSamplesTable>;I'm not sure this would be useful because these structures are fairly small.
There was a problem hiding this comment.
Also should the Counter type move to profile-derived?
There was a problem hiding this comment.
Also should the Counter type move to
profile-derived?
Oh, definitely, I missed that.
There was a problem hiding this comment.
So, now that I've moved the type to profile-derived, I think I want to keep the duplication. I'd like people who want to generate their own profiles to see very straightforward type definitions in types/profile.js.
| export function getSampleIndexRangeForSelection( | ||
| table: { time: Milliseconds[], length: number }, | ||
| times: { time: Milliseconds[], length: number }, |
There was a problem hiding this comment.
(optional) This function getSampleIndexRangeForSelection looks pretty useless atm, maybe it can be simplified by calling it directly with the time property?
(same for getInclusiveIndexRangeForSelection below)
There was a problem hiding this comment.
Yeah it's somewhat useless. It has 9 callers though so at least it's not entirely useless. I'll leave it for now, but not opposed to your suggestion.
| // Change the root range for testing. | ||
| const samples = profile.threads[0].samples; | ||
| samples.time[samples.length - 1] = 50; | ||
| ensureExists(samples.time)[samples.length - 1] = 50; |
There was a problem hiding this comment.
This is the kind of place where we'd like typescript's ! operator :-)
| } | ||
| } else if (samplesTimeDeltasCol !== undefined) { | ||
| for (let i = 1; i < samples.length; i++) { | ||
| const sampleTimeDeltaInMs = samplesTimeDeltasCol[i]; |
There was a problem hiding this comment.
The only difference is how we gather the delta, it might be worth either extracting this part into a function (the only different part for the 2 cases) or computing the diffs first (in that case we'd loop twice in the first case, but maybe that's OK?).
I always find it hard when I look at similar-but-subtly-different pieces of code to actually tell what's different.
If you think that would be too complex for what we need, alternatively you can add comments on top of the different lines.
There was a problem hiding this comment.
Good point, I'll change it to compute the deltas first and then have shared code for the loop.
There was a problem hiding this comment.
Changed to:
const timeDeltas =
samples.time !== undefined
? timeColumnToTimeDeltas(samples.time)
: ensureExists(samples.timeDeltas);
// Ignore the first CPU delta value; it's meaningless because there is no
// previous sample.
for (let i = 1; i < samples.length; i++) {
const sampleTimeDeltaInMs = timeDeltas[i];
if (sampleTimeDeltaInMs !== 0) {
const cpuDeltaPerMs = (threadCPUDelta[i] || 0) / sampleTimeDeltaInMs;
maxThreadCPUDeltaPerMs = Math.max(
maxThreadCPUDeltaPerMs,
cpuDeltaPerMs
);
}
}| const timeDeltas = new Array(time.length); | ||
| let prevTimeNs = 0; | ||
| for (let i = 0; i < time.length; i++) { | ||
| const currentTimeNs = Math.round(time[i] * NS_PER_MS); |
There was a problem hiding this comment.
I'm a bit worried that by multiplying by 1_000_000 we get closer to the max safe integer.
(although this seems to be ~2501 hours so it's probably OK);
A comment explaining why you're doing this dance with ns and round would be good!
There was a problem hiding this comment.
Good point, I'll add a comment. I added this because otherwise a lot of the numbers in the snapshot tests became longer.
Hrmmm and you're right about max safe integer being a bit of a problem. For example, Math.round(9007199254.741003 * 1000000) / 1000000 === 9007199254.741005 and 9007199254.741005 !== 9007199254.741003. I don't think it matters in practice but I can't prove it at the moment.
There was a problem hiding this comment.
I'm adding the following comment:
// For each timestamp in the time series, compute the delta to the previous
// timestamp. The implicit initial timestamp is zero.
//
// Timestamps are in milliseconds. To compute the deltas, we first convert each
// timestamp to integer nanoseconds. Then we subtract those nanosecond timestamps
// and converting the delta to milliseconds again. We do this dance so that
// the deltas have a "compact" stringified representation. Otherwise,
// converting to deltas could easily increase the JSON size.
// For example, 252.728334 - 240.520375 === 12.207958999999988.| samples: RawSamplesTable | RawCounterSamplesTable | ||
| ): number[] { | ||
| const { time, timeDeltas } = samples; | ||
| return time ?? computeTimeColumnFromTimeDeltas(ensureExists(timeDeltas)); |
There was a problem hiding this comment.
optional: computeTimeColumnFromTimeDeltas is used only here, it might be good to inline it here
There was a problem hiding this comment.
I'm keeping it separate, no strong opinions either way though
julienw
left a comment
There was a problem hiding this comment.
Thanks, this is pretty good work! And probably very tedious too.
For the 3 first commits I only have small comments as you'll see.
As a more general comment, and I know it's more a matter of preference, I think I would have liked to see the derived types really have Derived in their names, such as DerivedThread, DerivedCounter, etc. You don't need to do it now in this PR but I'm curious to know if you'd agree.
For the last commit (about time and timeDeltas), all these ensureExists for the time and timeDeltas properties in the RawSamplesTable, as well as the numerous if conditions, seem cumbersome to me, and also the type is error prone and makes it necessary to have runtime checks.
It may be easier to use timeDeltas as a mandatory property in RawSamplesTable, but support both at load time only. This would mean having just one call to convert time (if present) to timeDeltas in unserializeProfileOfArbitraryFormat, but otherwise deal with timeDeltas being always present in the RawProfile type.
What do you think?
I'd be fine with this change in a follow-up PR so that we can move forward, but it might be easier to do it know so that the patch doesn't add all the ensureExists.
I'm approving for this reason but please request a new review if you do any substantial changes. Please also add substantial changes to extra commits (github makes it difficult to see updates of existing commits).
Thanks again, I'm excited about these changes!
| function makeSamplesUseTimeDeltas(samples) { | ||
| const NS_PER_MS = 1000000; | ||
| const { time } = samples; | ||
| const timeDeltas = new Array(time.length); | ||
| let prevTimeNs = 0; | ||
| for (let i = 0; i < time.length; i++) { | ||
| const currentTimeNs = Math.round(time[i] * NS_PER_MS); | ||
| timeDeltas[i] = (currentTimeNs - prevTimeNs) / NS_PER_MS; | ||
| prevTimeNs = currentTimeNs; | ||
| } | ||
| samples.timeDeltas = timeDeltas; | ||
| delete samples.time; | ||
| } | ||
|
|
||
| for (const thread of profile.threads) { | ||
| makeSamplesUseTimeDeltas(thread.samples); | ||
| } | ||
|
|
||
| const counters = profile.counters; | ||
| if (counters) { | ||
| for (const counter of counters) { | ||
| makeSamplesUseTimeDeltas(counter.samples); | ||
| } | ||
| } |
There was a problem hiding this comment.
Initially I was confused about the fact you're doing that in v50 instead of adding a new upgrader.
My understanding that you're adding this in the upgrade for v50 is that all profiles uploaded after v50 should already have their data as timeDeltas.
But because we support both formats we might not need an upgrader at all?
If that's only for reducing the size of the profiles that would be reuploaded, please at least add a comment about that. But because I believe that reuploading profiles is pretty rare my preference would be that we don't update them.
Unless we make timeDeltas encoding mandatory for RawProfile (but still support both time and timeDeltas at load time). But I think that's not the path you took here?
There was a problem hiding this comment.
Initially I was confused about the fact you're doing that in v50 instead of adding a new upgrader.
I bet! I had to justify it to myself a few times and I wrote something about it into the commit message, too.
My understanding that you're adding this in the upgrade for v50 is that all profiles uploaded after v50 should already have their data as timeDeltas. But because we support both formats we might not need an upgrader at all?
That's true, we don't "need" it. I was mostly doing it to keep the behavior we already had.
If that's only for reducing the size of the profiles that would be reuploaded, please at least add a comment about that. But because I believe that reuploading profiles is pretty rare my preference would be that we don't update them.
You're right, it would only benefit re-uploaded profiles. Ok, I am convinced.
Unless we make timeDeltas encoding mandatory for RawProfile (but still support both time and timeDeltas at load time). But I think that's not the path you took here?
Correct.
I will remove it and adjust the commit message.
| /** | ||
| * The derived samples table. | ||
| */ | ||
| export type SamplesTable = {| |
There was a problem hiding this comment.
I think I would have used the same thing than before with $Diff to avoid some duplication, but I'm not sure it's better. Omit would be much easier to use but it's not in our version of Flow. So I'm fine with the duplication.
There was a problem hiding this comment.
I like the duplication in this case because it makes it clear that the types are completely independent and you don't need to worry about the other type when changing one of them.
I felt this way too, in the beginning. I think the reason I changed my mind was because the derived types are supposed to be the more ergonomic types, and typing "Derived" is not very ergonomic. I'm going to keep it as-is, and we can live with it for a bit, and then decide in a few weeks if we want to rename the derived types to Derived. I wouldn't be sad if we did. But maybe we'll find that we don't need it. |
I agree that these
I don't like your suggestion of making the deltas representation mandatory in the We could make the deltas representation mandatory in the file format, too. I'm not really sure about the benefits / drawbacks of this. In this PR I didn't want to make changes in the accepted format. I think it's ok to give profile generators some freedom in how to represent a time series; for example, at some point we were talking about having a way to set a time granularity, so that very long profiles which don't care about millisecond accuracy can express those timestamps in a compact way, too. But we should find a way to make it more ergonomic. One idea I had was to have something like this: and then those typed arrays would only be used when parsing the binary profile format (assuming we make such a format) and not when parsing the JSON representation of the file. And then we'd have functions like As for writing the values, I think we just need some some helper classes to create these tables. For example, a |
511eeaa to
e1f1a3f
Compare
This removes the distinction between the "serializable" and the "unserialized" / in-memory format. The raw profile is now always serializable to JSON. The format stays unchanged - samples and counter samples are still allowed to have their timestamps stored as either timestamps or as time deltas. We now convert the samples and counter samples into the timeDeltas format during processing. This matches the previous behavior where we did this conversion during profile serialization. One behavior change is that, if you open an existing processed profile that uses absolute timestamps, and reupload it, the reuploaded profile will keep using absolute timestamps, whereas before this change, the reuploaded profile would use time deltas.
e1f1a3f to
4490654
Compare
Updates: [Julien Wajsberg] Some more small refactorings (#5320) [Markus Stange] Pass the correct sample index offset to getTimingsForCallNodeIndex for the flame graph tooltip. (#5328) [Nisarg Jhaveri] Update docs to include Android Studio/Simpleperf trace file support (#5309) [Markus Stange] Don't pass the preview filtered thread to getTimingsForPath/CallNodeIndex. (#5329) [Nazım Can Altınova] Add a "Sample timestamp" field to the sample tooltip in timeline (#5322) [Markus Stange] Reduce confusion between call tree summary strategy aware samples and regular samples (#5330) [Markus Stange] Rename this getCounter selector to getCounters. (#5337) [Markus Stange] Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strat> [Markus Stange] Remove some code that uses the preview filtered thread (#5336) [Markus Stange] Remove getMarkerSchemaName special cases - look up marker schemas from data.type and nothing else (#5293) [Markus Stange] Remove the makeProfileSerializable step - make the raw in-memory profile match the format that's stored in the file (#5287) [Nicolas Chevobbe] Adapt FilterNavigatorBar to High Contrast Mode. (#5257) [Nicolas Chevobbe] Adapt Tracks to High Contrast Mode. (#5252) [Markus Stange] Adjust string index fields in markers when merging threads (#5344) [Theodoros Nikolaou] Localize title and aria label in ProfileName (#5345) [Julien Wajsberg] Adapt time-slice selection in High Contrast Mode. (#5259) [Markus Stange] Make stackTable (sub)category derived data (#5342) [Markus Stange] Compute cpuRatio values when computing the derived thread (#5288) [Nazım Can Altınova] Add a context menu item to open the JS scripts in DevTools debugger (#5295) Also thanks to our localizers: el: Jim Spentzos fr: Théo Chevalier it: Francesco Lodolo [:flod] zh-TW: Pin-guang Chen
This sequence of patches makes it so that the processed format is the same as the serializable processed format. There is no additional step when converting the profile from/to JSON.
Instead, we introduce a RawThread and make Thread a derived type. RawThreads are stored in
profile.threads. Threads are computed by redux selectors. The conversions that happened during unserialization now happen when computing the derived thread.RawThreadhastimeDeltascolumns,Threadhastimecolumns.RawThreadhas astringArray,Threadhas astringTable.Having a derived
Threadtype will also make it easier to share data between threads, because we can put the shared data (e.g. the shared string table) on the derived thread and don't have to change all the code that accessesthread.stringTable.