Skip to content

Move call node path/index conversion functions into CallNodeInfo#4896

Merged
mstange merged 1 commit into
firefox-devtools:mainfrom
mstange:make-convert-methods-on-callnodeinfo
Jan 23, 2024
Merged

Move call node path/index conversion functions into CallNodeInfo#4896
mstange merged 1 commit into
firefox-devtools:mainfrom
mstange:make-convert-methods-on-callnodeinfo

Conversation

@mstange

@mstange mstange commented Jan 22, 2024

Copy link
Copy Markdown
Contributor

This makes CallNodeInfo an interface, and puts the conversion functions between call node indexes and call node paths onto this new interface. It also provides accessor methods for the traditional members.

This reduces the number of direct uses of the call node table. It will allow us to have two implementations in the future: One for the non-inverted call tree, and one for the inverted call tree.

@mstange mstange requested a review from julienw January 22, 2024 21:34
@mstange mstange self-assigned this Jan 22, 2024
@codecov

codecov Bot commented Jan 22, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (5ab5512) 88.41% compared to head (4c52032) 88.42%.

❗ Current head 4c52032 differs from pull request most recent head 8ee921a. Consider uploading reports for the commit 8ee921a to get more accurate results

Files Patch % Lines
src/components/flame-graph/FlameGraph.js 71.42% 2 Missing ⚠️
src/components/stack-chart/index.js 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4896      +/-   ##
==========================================
+ Coverage   88.41%   88.42%   +0.01%     
==========================================
  Files         303      304       +1     
  Lines       27133    27177      +44     
  Branches     7334     7332       -2     
==========================================
+ Hits        23989    24032      +43     
- Misses       2926     2927       +1     
  Partials      218      218              

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

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty solid! Now I want to see what comes next :-)

(nothing to do for the following comment)
I noticed some places where we pass a callNodeInfo but only the CallNodeTable is used. Maybe something we could make better later? Although I guess a change to callNodeInfo is equivalent to a change to a callNodeTable, given how they're generated, so probably not worth changing. Happy to hear your thoughts on that.

Comment thread src/profile-logic/call-node-info.js
@mstange

mstange commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

I noticed some places where we pass a callNodeInfo but only the CallNodeTable is used. Maybe something we could make better later? Although I guess a change to callNodeInfo is equivalent to a change to a callNodeTable, given how they're generated, so probably not worth changing. Happy to hear your thoughts on that.

Oh, you mean for tighter memoization? Yeah in that case I agree with your "although" part - they're always regenerated together, so it shouldn't make a difference for memoization purposes.

I think after the rest of the fast-invert changes, not many functions will be left which only use the CallNodeTable and ignore the rest of the CallNodeInfo. We can do another pass at the end of this work to see if there are spots I missed and that can be simplified.

This makes CallNodeInfo an interface, and puts the conversion functions
between call node indexes and call node paths onto this new interface.
It also provides accessor methods for the traditional members.

This reduces the number of direct uses of the call node table.
It will allow us to have two implementations in the future: One for the
non-inverted call tree, and one for the inverted call tree.
@mstange mstange force-pushed the make-convert-methods-on-callnodeinfo branch from 4c52032 to 8ee921a Compare January 23, 2024 16:08
@mstange mstange enabled auto-merge January 23, 2024 16:08
@mstange mstange merged commit 0f047ca into firefox-devtools:main Jan 23, 2024
@julienw julienw mentioned this pull request Feb 6, 2024
julienw added a commit that referenced this pull request Feb 6, 2024
[@canova Nazım Can Altınova] Return null instead of throwing an error if it fails to get the resource name in active tab view (#4905)
[@fqueze Florian Quèze] Improve the precision of the getFittedText implementation. (#4893)
[@fqueze Florian Quèze] Show marker count next to the marker name in the marker chart when a marker is hovered. (#4892)
[@mstange Markus Stange] Convert some code to use the non-inverted call node table even while we're showing the inverted call tree (Merge PR #4899)
[@canova Nazım Can Altınova] Bring the max stacking depth limit back for marker timings (#4898)
[@mstange Markus Stange] Various preparations in call tree code for fast inverted tree (Merge PR #4897)
[@mstange Markus Stange] Move call node path/index conversion functions into CallNodeInfo (Merge PR #4896)
[@mstange Markus Stange] Make mergeFunction fast (Merge PR #4895)
[@gthb Gunnlaugur Thor Briem] feat: support from-url profiles in compare (#4875)

And all the locale contributors:
de: Michael Köhler
el: Jim Spentzos
en-GB: Ian Neal
es-CL: ravmn
fr: Théo Chevalier
fy-NL: Fjoerfoks
ia: Melo46
it: Francesco Lodolo [:flod]
nl: Fjoerfoks, Mark Heijl
pt-BR: Marcelo Ghelman
ru: Valery Ledovskoy
sv-SE: Andreas Pettersson
uk: Artem Polivanchuk
zh-CN: 你我皆凡人, Olvcpr423, Pontoon
zh-TW: Pin-guang Chen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants