New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ranged filtered markers in marker chart #865
Use ranged filtered markers in marker chart #865
Conversation
const getMarkerTiming = createSelector( | ||
getRangeSelectionFilteredTracingMarkers, | ||
MarkerTiming.getMarkerTiming | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the important change: we loop over marker timings in the chart. Even if we don't draw all of them, if we have a lot of markers merely looping through them can be costly.
The obvious drawback is that we'll have to regenerate the marker timings at each range change. I'd like that you measure the effect of changing the range on this. Maybe it's less costly to filter the marker timings using the range, rather than rebuild the marker timings from the markers filtered with the range (if you follow me). But I really don't know so we need to measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for sure this is better to do this filtering here, in memoized selectors, rather than do it again and again in the react component.
5beec84
to
caf9103
Compare
Codecov Report
@@ Coverage Diff @@
## master #865 +/- ##
==========================================
+ Coverage 70.89% 70.94% +0.04%
==========================================
Files 141 141
Lines 8325 8325
Branches 1895 1895
==========================================
+ Hits 5902 5906 +4
+ Misses 2140 2137 -3
+ Partials 283 282 -1
Continue to review full report at Codecov.
|
79cde94
to
48e3d2d
Compare
48e3d2d
to
86a51d2
Compare
86a51d2
to
054bf0d
Compare
@julienw This is ready for review. I added a test for it. I also don't think the perf of running the selectors is critical as it's not on a hot path, and doesn't feel slow when I use the tool. I didn't include a profile of that as you commented. |
@julienw looks like the ball is in your court on this one. I still need to rebase it. |
Ah nice ! I don't remember why I missed it. I even feel like I looked at it already... anyway this is in my TODO now. |
I'd like to look again at how this looks like after #1134 lands. I think this is nice for the UI (less empty lines when we don't need them) but that it doesn't change much the performance as our performance in this chart is mainly driven by how much we draw to the canvas (which is what #1134 fixes in some situations, and what your patch was incidentally fixing). |
I took some measurements with #1134:
The main benefit without this patch is that the MarkerTimings need to be computed only once at load time. The downside is that we need to compute them for all markers even if we display a part of it. And now with both #1134 and this patch:
When using it for real there doesn't seem to be very different performance characteristics with this PR. Although I'd like to understand why UX difference In terms of UX I prefer the behavior with this patch. Compare without the patch: So I find the second behavior much cleaner. |
Closing in favor of #1216. |
This profile has a large amount of markers and is slow on rendering the marker chart. I switched it to use the
getRangeSelectionFilteredTracingMarkers
and things got fast again. The tests need fixing for this, so it's still WIP.STR: Mouse over the markers in the marker chart.
Before:
After: