Change StringTable API a bit. by mstange · Pull Request #5286 · firefox-devtools/profiler

mstange · 2025-01-02T21:18:17Z

This avoids copies of the underlying array.

It also provides a new "constructor" (StringTable.withBackingArray) which avoids recreating the map when we need a StringTable for the same array in different places.

codecov · 2025-01-02T21:26:55Z

Codecov Report

Attention: Patch coverage is 97.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.62%. Comparing base (b3fe3c5) to head (e8a5f06).

Files with missing lines	Patch %	Lines
src/profile-logic/processed-profile-versioning.js	92.85%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5286   +/-   ##
=======================================
  Coverage   88.61%   88.62%           
=======================================
  Files         308      308           
  Lines       28125    28114   -11     
  Branches     7621     7618    -3     
=======================================
- Hits        24923    24916    -7     
+ Misses       2987     2984    -3     
+ Partials      215      214    -1

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

julienw · 2025-01-07T10:50:28Z

I'm not opposed, but to avoid copies, I would have rather done a new static "constructor" taking a StringTable, not the backing array. From this static function it would be allowed to access the underlying array, but keeping the encapsulation of the array (and the synchronization with the map). This would also have skipped all the changes in the current use.

mstange · 2025-01-09T17:07:07Z

I'm not opposed, but to avoid copies, I would have rather done a new static "constructor" taking a StringTable, not the backing array.

Hmm, I'm not sure I understand your proposal. For example, in the upgraders, there are multiple upgraders that interact with the stringArray through a StringTable. Where would the second upgrader get the StringTable from the first upgrader?

julienw · 2025-01-10T15:04:12Z

Ah yes, I see, for these cases the new factory function makes a lot of sense.

Maybe I'd be happy enough with keeping the usage of the main constructor where its usage is not a problem.

But again I'm not opposed to these changes so I'm also fine with keeping them like that.

Now I guess I'm the reviewer :-) I'll look at it more closely soon

julienw

As I wrote in the previous comment, I would mildly prefer to keep using the constructor in the cases it's not a problem (mostly when it's used without a parameter, as an empty table). But I'm also fine with your approach of replacing all of its uses.

Otherwise there's just one simple thing to improve IMO before landing it, see the comment below!

julienw · 2025-01-10T15:56:00Z

    thread.tid = i;
    thread.name = name;
-    thread.stringTable = new StringTable(stringTable.serializeToArray());
+    thread.stringTable = stringTable;


I wondered about that a bit: is it a problem that each thread have the same instance. But I guess it's fine.

Yeah this is indeed a change in behavior, but I agree it should be fine. And soon (hopefully) we will have just one stringTable across all threads anyway.

julienw · 2025-01-10T15:59:20Z

+  /**
+   * Exposes the underlying array. Do not mutate the contents.
+   */
+  getBackingArray(): string[] {


return $ReadOnlyArray<string> so that Flow prevents mutating the items.

I tried this but ran into an issue that I didn't know how to address: makeProfileSerializable calls getBackingArray() and stores the array in the SerializableThread's stringArray. So to make this work I would need to change the type of stringArray to $ReadOnlyArray<string>. But if I do that, then I break _unserializeProfile! Because it passes stringArray to StringTable.withBackingArray, which definitely cannot take a $ReadOnlyArray<string>.

interesting, I guess this means we should have 2 types of SerializedThread, depending whether it's incoming or outgoing :-)

We should have zero copies of it. #5287

Sorry, that was a bit on the flippant side. What I mean is: True, that would be a way to fix it! And it would express the intent well. But since I'm planning to get rid of the SerializedThread type anyway, I don't want to put too much effort into writing code that'll be deleted soon.

Ah yeah, I should also have been explicit that I didn't want to do that now, it was more of a general thought.

This avoids copies of the underlying array. It also provides a new "constructor" (StringTable.withBackingArray) which avoids recreating the map when we need a StringTable for the same array in different places.

mstange · 2025-01-13T22:28:51Z

As I wrote in the previous comment, I would mildly prefer to keep using the constructor in the cases it's not a problem (mostly when it's used without a parameter, as an empty table). But I'm also fine with your approach of replacing all of its uses.

I would actually agree with you if all I had seen was this PR. But I'm going to leave it as I had it because it makes the stringArray commit in #5287 a bit simpler, I think. But I'm happy to revisit it in #5287.

Updates: [Nicolas Chevobbe] Adapt Keyboard shortcut dialog in High Contrast Mode. (#5245) [Nicolas Chevobbe] Fix sidebar-toggle in High Contrast Mode. (#5246) [Nicolas Chevobbe] Fix timeline selection overlay time / hover line in High Contrast Mode (#5247) [Zac Spitzer] fix broken link for processed profile format (#5267) [Greg Tatum] Update the memory allocation docs and add DHAT docs (#5270) [Markus Stange] Simplify some code related to thread CPU deltas (#5265) [Greg Tatum] Update dhat convertor to work better with valgrind (#5269) [Markus Stange] Rename UniqueStringArray to StringTable. (#5283) [Markus Stange] Use snapshot testing in the symbolicator CLI test. (#5284) [Markus Stange] Fix two confused upgraders which didn't expect to be run on the serialized format. (#5285) [Nicolas Chevobbe] Fix timelineSettingsHiddenTracks in High Contrast Mode. (#5250) [Julien Wajsberg] Fix some test and non-test warnings (#5294) [Nisarg Jhaveri] Support importing simpleperf trace files from Android Studio (#5212) [Sean Kim] Add HTTP response status code in the profiler network tab (#5297) [Markus Stange] Change StringTable API a bit. (#5286) [Markus Stange] Correctly declare imported simpleperf trace profiles to be of the current version. (#5312) [Markus Stange] Two small changes (#5313) [Nazım Can Altınova] Show sample tooltips on sample graph hover (#5298) Also thanks to our localizers: en-CA: chutten es-CL: ravmn es-CL: ravmn fr: Théo Chevalier ia: Melo46 ia: Melo46 sv-SE: Andreas Pettersson uk: Lobodzets zh-TW: Olvcpr423

mstange requested review from canova and removed request for canova January 2, 2025 21:18

mstange marked this pull request as draft January 2, 2025 21:18

mstange force-pushed the push-otquwzrxynyz branch from 076bfcc to d51802f Compare January 2, 2025 21:20

mstange marked this pull request as ready for review January 2, 2025 21:20

mstange requested a review from canova January 2, 2025 21:20

mstange self-assigned this Jan 2, 2025

mstange force-pushed the push-otquwzrxynyz branch from d51802f to b57be6c Compare January 2, 2025 21:21

mstange force-pushed the push-otquwzrxynyz branch 2 times, most recently from 79a6cff to e8a5f06 Compare January 3, 2025 20:25

mstange requested review from julienw and removed request for canova January 9, 2025 18:37

julienw approved these changes Jan 10, 2025

View reviewed changes

Change StringTable API a bit.

ed9ff7f

This avoids copies of the underlying array. It also provides a new "constructor" (StringTable.withBackingArray) which avoids recreating the map when we need a StringTable for the same array in different places.

mstange force-pushed the push-otquwzrxynyz branch from e8a5f06 to ed9ff7f Compare January 13, 2025 22:29

mstange merged commit 1bd1609 into firefox-devtools:main Jan 13, 2025

julienw mentioned this pull request Jan 15, 2025

Deploy January 15, 2025 #5318

Merged

Uh oh!

Conversation

mstange commented Jan 2, 2025

Uh oh!

codecov Bot commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julienw commented Jan 7, 2025

Uh oh!

mstange commented Jan 9, 2025

Uh oh!

julienw commented Jan 10, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstange commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jan 2, 2025 •

edited

Loading