Share stackTable, frameTable, funcTable, resourceTable and nativeSymbols between threads

[mstange]

Production | Deploy preview

Sharing the string table happened in #5481.
This PR makes us share the rest, and completes issue #3918.

Only samples and markers are still per-thread. I don't expect this to change.
This includes allocation samples.

To do:

• Fix type check and test failures
• Upgrade the URL when upgrading the profile, to remap function indexes etc. in the URL
• When compacting during upload, apply translation tables to redux state (e.g. update focus function indexes in URL)
• Do something to address perf impact for computing the derived thread (transforms and call node table computation happen per thread and take longer if the tables are bigger because they now contain information for all threads, should be able to reuse more of the computed outputs in the thread derivation pipeline)
• Memoize the computation of the implementation filtered thread across threads
• Adjust indexes in transforms when merging two different profiles
• Add a paragraph to CHANGELOG-formats.md for the new version.
• Add a test for merging profiles with applied transforms where we actually check the filtered call tree

Potential follow-ups:

• Add a test checking that IndexIntoSourceTable indexes in the redux state are updated after profile sanitizing
• Add a test checking that collapse-resource transforms aren't broken if profile sanitizing adds elements to the funcTable

[vlasakm]

This is really great! Sharing this data across threads works especially well for applications where there is a large number of threads, but just a few "thread pools" whose threads are all executing the same code.

Using this allowed me to employ better caching when creating a firefox profile, save on the profile size, etc.

It also made me greedy, and I attempted to visualize a rather large profile (1602 threads, 577108 stacks, 29760 frames). Previously my experience was that Firefox Profiler would choke on say more than 300+ threads, and I hoped this change with sharing could improve the situation. Now it loads, and the UI is still quite responsive, but the load of the profile is taking long 2 minutes 15 seconds (and Firefox prompts to kill the script, etc.).

90 % of the load time is spent is in computeCallNodeTable, which I believe may be called repeatedly for each thread (and the merged thread which includes all of them), even though all these calls would process the same stack table and frame table, now that they are shared.

I know very little about React/Redux, but I see that there's some concept of memoization and thread selectors, that is still used for call nodes, even though after this change to share the stack and frame table. Very naively, would it perhaps be possible to just compute and memoize the call node information once by not using the thread selector mechanism here? But there's a great chance I am missing all the needed context.

FWIW I was and am still using this branch for quite some time. I am not sure if I am able to contribute much (I even failed to rebase it on top of more recent changes), but it would be great to see this, perhaps after all the big work-in-progress changes land (like the Typescript migration).

[mstange]

Yes indeed, sharing the computed call node table across threads while properly handling per thread transforms is the next thing I want to work on when I get back to this task. Thanks for the encouragement and it's great to hear that you're seeing real improvements from it!

[codecov]

Codecov Report

❌ Patch coverage is 85.69092% with 175 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.36%. Comparing base (19aa5e9) to head (850e4ff).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
src/app-logic/url-handling.ts	12.50%	63 Missing ⚠️
src/profile-logic/transforms.ts	29.41%	48 Missing ⚠️
src/profile-logic/profile-compacting.ts	87.96%	25 Missing and 1 partial ⚠️
src/profile-logic/sanitize.ts	88.60%	9 Missing ⚠️
src/reducers/code.ts	50.00%	6 Missing ⚠️
src/profile-logic/merge-compare.ts	96.66%	5 Missing ⚠️
src/profile-logic/profile-data.ts	94.73%	5 Missing ⚠️
src/reducers/url-state.ts	80.00%	5 Missing ⚠️
src/profile-logic/processed-profile-versioning.ts	97.87%	3 Missing ⚠️
src/profile-logic/index-translation.ts	90.47%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5482      +/-   ##
==========================================
- Coverage   85.56%   85.36%   -0.20%     
==========================================
  Files         319      320       +1     
  Lines       31499    31916     +417     
  Branches     8697     8780      +83     
==========================================
+ Hits        26953    27246     +293     
- Misses       4115     4239     +124     
  Partials      431      431              

☔ 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.

[mstange]

This is now ready for review.

[canova]

I promised myself that I would be done with reviewing this PR today, but I underestimated the amount of changes here 😅 I'm done-ish with the first commit, which is the main beast. So pushing the review comments now even though I still need to review the others.

[canova]

I'm done reviewing the rest of the PR! (I saw the github's unicorn error message quite a few times while doing so 😄) It was a lot easier to review the other commits. I think it looks good to me with the issues I raised yesterday fixed. I would prefer to take a look at the PR once more once these are fixed though. But I assume that review will be a lot more quicker.

[canova]

Ah another question, I see that there are 2 items unchecked in the list. Are they done? I think we can also do those as follow-ups as they don't look critical.

[mstange]

Ah another question, I see that there are 2 items unchecked in the list. Are they done? I think we can also do those as follow-ups as they don't look critical.

Ah yes, I took a brief look at this and it didn't look straightforward to add those tests, so I'll leave those two items for potential follow-ups. Edited the PR description.

[canova]

Thanks, looks great! Let's land it before it gets more bitrotted 😄

[mstange]

Thanks for the review!