Share stackTable, frameTable, funcTable, resourceTable and nativeSymbols between threads#5482
Share stackTable, frameTable, funcTable, resourceTable and nativeSymbols between threads#5482mstange wants to merge 9 commits intofirefox-devtools:mainfrom
Conversation
|
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 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). |
|
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! |
6f1959d to
4e19f9e
Compare
24a4604 to
8d957f0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5482 +/- ##
==========================================
- Coverage 85.56% 85.37% -0.20%
==========================================
Files 319 320 +1
Lines 31499 31908 +409
Branches 8705 8687 -18
==========================================
+ Hits 26953 27241 +288
- Misses 4115 4236 +121
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a5b1248 to
4b18e81
Compare
4b18e81 to
ff1d089
Compare
Small things I noticed while working on #5482.
ff1d089 to
76f35fa
Compare
|
This is now ready for review. |
ce546d0 to
0d1cc61
Compare
More small stuff that makes #5482 a tiny bit easier.
d1916d3 to
cf780f4
Compare
canova
left a comment
There was a problem hiding this comment.
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.
| profile: Profile | null, | ||
| browserConnection: BrowserConnection | null | ||
| browserConnection: BrowserConnection | null, | ||
| profileUpgradeInfo?: ProfileUpgradeInfo |
There was a problem hiding this comment.
Oh, I'm a bit surprised with the profileUpgradeInfo, but seems fine.
There was a problem hiding this comment.
Yeah I'll go ahead and add some comments about the profileUpgradeInfo business.
There was a problem hiding this comment.
Oh interesting, it looks like I was using it as an outparameter here. I think I will add a commit to refactor this a bit.
| const resourceIndex = funcIndex - threadMappings.funcTableIndexMap.length; | ||
| if (resourceIndex in threadMappings.resourceTableIndexMap) { | ||
| return ( | ||
| newFuncCount + threadMappings.resourceTableIndexMap[resourceIndex] | ||
| ); | ||
| } | ||
| return funcIndex; |
There was a problem hiding this comment.
Hmm, I'm confused by this part of this function. How do we compute the resource index?
Edit: I think I understand more now, I missed that the if (funcIndex in threadMappings.funcTableIndexMap) { first if was returning early if the funcIndex is in the map. And this is like a special case for collapse-resource transform. I think it would be great to have a comment here explaining the behavior.
There was a problem hiding this comment.
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.
|
|
||
| function _gatherSourceReferencesInProfile(profile: Profile): Uint8Array { | ||
| const referencedSources = new Uint8Array(profile.shared.sources.length); | ||
| _gatherReferencesInStackTable(shared.stackTable, referencedSharedData); |
There was a problem hiding this comment.
Would it make sense to mention that the order of these calls are important, for example _gatherReferencesInSources requires referencedSources to be populated in _gatherReferencesInFuncTable
| type TranslationMapForStacks = Int32Array; | ||
| type TranslationMapForLibs = Int32Array; | ||
| type TranslationMapForStrings = Int32Array; | ||
| type TranslationMapForSources = Int32Array; |
There was a problem hiding this comment.
Nice! It's a lot better to use Int32Array instead of maps.
Can we also add a comment at the top of this block mentioning the oldXToNewXPlusOne behavior as it might not be obvious to everyone at first.
|
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. |
…Symbols across threads. This makes func indexes etc valid globally instead of scoped per threads. It also allows for smaller profiles because data doesn't have to be duplicated. Symbolication is probably faster. Selecting multiple threads, i.e. displaying a "merged thread", is now easier because we no longer need to compute merged funcTables etc for the merged thread; the global tables are already "merged". This also also means that func index mapping during symbolication now works even if multiple threads are selected. Fixes firefox-devtools#5703. This profile version comes both with a profile upgrader and with a URL upgrader, because we have to adjust any func and resource indexes for transforms in the URL.
cf780f4 to
52e7600
Compare
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. |
We also need to adjust any indexes in the redux state after compacting, for example funcTable or resourceTable indexes used by transforms. This patch also makes us adjust the source index in the URL after compaction. Strictly speaking this fixes an existing bug, because the source table was already being compacted. It also fixes the bug where we weren't keeping track of the oldFuncCount properly when splitting functions into private-window and non-private-window instances, and weren't updating collapse-resource transforms correctly. Fixes firefox-devtools#5793.
We compute the call node table based on the filtered thread, per thread. But now that every thread contains the full stackTable with entries used by all threads, this computation has become much more expensive. But most of the time we are passing the same tables from the shared profile info, so we can just do the work once and share it across threads. This only fixes the case where none of the threads have a transform applied. They'll end up passing the tables from the shared data and hit the memoization cache. If a thread has a transform, it'll pass different tables and not hit the cache. We don't do anything to share work across two threads that happen to have the same transforms. This also doesn't fix the case where there's an implementation filter applied. Here's how loading the profile https://share.firefox.dev/4bxDqu8 compares: - Before sharing the tables: https://share.firefox.dev/4r0imRO (1.1 seconds) - After sharing the tables, no global memoization: https://share.firefox.dev/4qNGp6m (1.8 seconds) - After sharing the tables, with global memoization: https://share.firefox.dev/4rwkwso (1.3 seconds)
…threads. Here's how loading the profile https://share.firefox.dev/4kbkjrU compares: - Before sharing the tables: https://share.firefox.dev/4kh5i84 (1.1 seconds) - After sharing the tables, no global memoization: https://share.firefox.dev/3NLn1IK (1.6 seconds) - After sharing the tables, with global memoization: https://share.firefox.dev/3LNNuot (1.3 seconds)
Here's how loading the profile https://share.firefox.dev/4a0b88Y compares: - Before sharing the tables: https://share.firefox.dev/4qeiVGu (1.1 seconds) - After sharing the tables, no global memoization: https://share.firefox.dev/3NUEi27 (2.0 seconds) - After sharing the tables, with global memoization: https://share.firefox.dev/4qcDQty (1.3 seconds)
52e7600 to
31327b6
Compare
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:
Potential follow-ups: