-
Notifications
You must be signed in to change notification settings - Fork 242
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
Draft: Making test_cell_speed_with_long_text() faster #913
Conversation
55c5064
to
c017aa2
Compare
10f32ec
to
a9531e1
Compare
a9531e1
to
54dac02
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #913 +/- ##
==========================================
+ Coverage 93.23% 93.32% +0.09%
==========================================
Files 27 27
Lines 7684 7686 +2
Branches 1395 1393 -2
==========================================
+ Hits 7164 7173 +9
+ Misses 330 322 -8
- Partials 190 191 +1
☔ View full report in Codecov by Sentry. |
@andersonhc & @gmischler: do you want to review this before I merge it? 🙂 |
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.
These changes all look fine to me.
Given how you reduced the expected test time from 23 to 18 seconds, they seem to result in a roughly 20% speed up. It will be interesting to see how the flame graph changes with that to find further bottlenecks.
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.
Mapping directly unicode to glyph ID should give a performance similar to 2.7.4 and the way you implemented it doesn't impact shaping. I don't have any improvement to suggest at this point.
Thank you for your feedbacks! Merged 🙂 |
Sadly, this is in fact really not a stable metric: I fear that our pipelines "speed" / available CPU depend too much on the current load on the GitHub Actions system... |
This is a follow-up to #911
I made minor performance-oriented improvements to
SubsetMap
:dict.get(key)
instead ofkey in dict
+dict[key]
__hash__()
method._char_id_per_unicode
) to avoid repeated processings inSubsetMap.pick()
I also used this opportunity to remove all accesses to the private property
._map
from outside the
SubsetMap
class.cf. #907 for some context