Textualize / rich

Rich is a Python library for rich text and beautiful formatting in the terminal.
https://rich.readthedocs.io/en/latest/
MIT License
48.13k stars 1.69k forks source link

Bisect all the things! #3300

Closed rodrigogiraoserrao closed 4 days ago

rodrigogiraoserrao commented 4 months ago

This PR replaces a couple of searches with the module bisect.

The screenshot below shows Rich's benchmarks ran, on my machine, on v13.7.1 and on my changes. There were clear wins at the top from the optimisations in cells.py.

I boxed a region of things that stayed the same but whose underlying code was changed to fix the bug reported in #3299.

I also cut the code in Text.divide in half by using bisect. The Text.divide benchmarks show no change but that seems to have a positive impact in Text.split. (8% in this screenshot, 5% - 10% from the multiple times I ran the benchmarks while testing changes).

Screenshot 2024-03-07 at 17 35 09

I don't know if I'm supposed to commit and upload the benchmark results, so I didn't for now. I also don't know if “optimised some code” should go on the changelog so I didn't put it there for now.

Fixes #3298 Fixes #3299

rodrigogiraoserrao commented 4 months ago

I also tried optimising Segment.divide. Tried two slightly different implementations but the benchmarks showed that I was actually slowing down the function. I couldn't bring myself to completely throw away the code so I included them in the commit message of commit 7912306.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 95.23810% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 98.26%. Comparing base (aabfd16) to head (85c0419). Report is 48 commits behind head on master.

Files Patch % Lines
rich/cells.py 95.65% 1 Missing :warning:
rich/segment.py 90.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3300 +/- ## ========================================== - Coverage 98.30% 98.26% -0.05% ========================================== Files 74 74 Lines 8038 8020 -18 ========================================== - Hits 7902 7881 -21 - Misses 136 139 +3 ``` | [Flag](https://app.codecov.io/gh/Textualize/rich/pull/3300/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/Textualize/rich/pull/3300/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `98.26% <95.23%> (-0.05%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

willmcgugan commented 3 months ago

Just discovered that bisect key was added in 3.10.

However, I don't think we will need it. It will work with the tuples--no need to separate them.

rodrigogiraoserrao commented 3 months ago

It will work with the tuples--no need to separate them.

It works with tuples directly, yes. See 7a53a9f. The performance win becomes less significant, though. Originally the ratio was at 0.57 and now it's at 0.67:

Screenshot 2024-03-11 at 12 01 19

rodrigogiraoserrao commented 3 months ago

@willmcgugan do you need anything else from me on this PR?

willmcgugan commented 3 months ago

No, its good. Just need to find the time to review.

willmcgugan commented 4 days ago

Going to break this out in to smaller changes.