Skip to content
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

Merged
merged 3 commits into from Sep 6, 2023

Conversation

Lucas-C
Copy link
Member

@Lucas-C Lucas-C commented Sep 4, 2023

This is a follow-up to #911

I made minor performance-oriented improvements to SubsetMap:

  • in 2 places, use a single call to dict.get(key) instead of key in dict + dict[key]
  • define our own, fast, __hash__() method
  • add a cache (._char_id_per_unicode) to avoid repeated processings in SubsetMap.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

@Lucas-C Lucas-C changed the title Making test_cell_speed_with_long_text() faster Draft: Making test_cell_speed_with_long_text() faster Sep 4, 2023
@Lucas-C Lucas-C marked this pull request as draft September 4, 2023 16:52
@Lucas-C Lucas-C force-pushed the faster-text-rendering branch 3 times, most recently from 10f32ec to a9531e1 Compare September 4, 2023 21:22
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 89.65% and project coverage change: +0.09% 🎉

Comparison is base (6e3334f) 93.23% compared to head (4a27a86) 93.32%.

❗ Current head 4a27a86 differs from pull request most recent head 54dac02. Consider uploading reports for the commit 54dac02 to get more accurate results

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     
Files Changed Coverage Δ
fpdf/fonts.py 95.33% <88.88%> (+3.03%) ⬆️
fpdf/output.py 97.18% <100.00%> (ø)

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

@Lucas-C Lucas-C marked this pull request as ready for review September 5, 2023 13:52
@Lucas-C
Copy link
Member Author

Lucas-C commented Sep 5, 2023

@andersonhc & @gmischler: do you want to review this before I merge it? 🙂
Any other suggestions to make this code faster?

Copy link
Collaborator

@gmischler gmischler left a 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.

Copy link
Collaborator

@andersonhc andersonhc left a 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.

@Lucas-C Lucas-C merged commit c4b445b into master Sep 6, 2023
10 checks passed
@Lucas-C Lucas-C deleted the faster-text-rendering branch September 6, 2023 11:25
@Lucas-C
Copy link
Member Author

Lucas-C commented Sep 6, 2023

Thank you for your feedbacks!

Merged 🙂

@Lucas-C
Copy link
Member Author

Lucas-C commented Sep 6, 2023

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.

Sadly, this is in fact really not a stable metric:
https://github.com/py-pdf/fpdf2/actions/runs/6097064580/job/16543945606
😞

I fear that our pipelines "speed" / available CPU depend too much on the current load on the GitHub Actions system...

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.

None yet

4 participants