Skip to content

Comments

PERF: Skip kwargs normalization in Artist._cm_set#31000

Merged
timhoffm merged 5 commits intomatplotlib:mainfrom
scottshambaugh:artist_cm_set
Feb 3, 2026
Merged

PERF: Skip kwargs normalization in Artist._cm_set#31000
timhoffm merged 5 commits intomatplotlib:mainfrom
scottshambaugh:artist_cm_set

Conversation

@scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Jan 19, 2026 •
edited
Loading

PR summary

_cm_set is called during every Text.draw() to handle text wrapping, but most text has wrapping disabled so _get_wrapped_text() returns the original text unchanged. The old code would always set and restore the value, but we can safely bypass that if the updated value is identical to the current.

This bit of code was using 5.5% of total draw time in my benchmark script that renders an empty plot, now dropped down to 1%
image

import time
import matplotlib.pyplot as plt

print("Starting...")

fig = plt.figure()
ax = fig.add_subplot(111, projection='3d')

print("Timing...")

start_time = time.perf_counter()
for i in range(100):
    ax.view_init(elev=i, azim=i)
    fig.canvas.draw()
end_time = time.perf_counter()
plt.close()

print(f"Time taken: {end_time - start_time:.4f} seconds")

PR checklist

@scottshambaugh scottshambaugh force-pushed the artist_cm_set branch 3 times, most recently from 91ab3d8 to 340d63c Compare January 19, 2026 23:26
@scottshambaugh scottshambaugh marked this pull request as ready for review January 19, 2026 23:47
@scottshambaugh scottshambaugh marked this pull request as draft January 20, 2026 01:02
@scottshambaugh scottshambaugh marked this pull request as ready for review January 20, 2026 01:52
@scottshambaugh scottshambaugh mentioned this pull request Jan 20, 2026
Closed
@QuLogic
Copy link
Member

QuLogic commented Jan 20, 2026

WRT #30979, since _cm_set is internal, I think we can assume that input is normalized and any calls that use aliases are bugs. I would even go so far as to make it an error, but adding that check might lose performance gains.

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jan 20, 2026 •
edited
Loading

Looking at it again I think getattr actually does raise if it doesn't locate the target attribute, so we should be protected. I updated to use _internal_update directly without the normalization.

anntzer
anntzer requested changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@anntzer anntzer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, for the specific case of text drawing, there's a much simpler solution: since #25346, the whole _cm_set() can be removed, because _get_layout() itself will use the wrapped text (this passes all test_text tests and the relevant wrapped text example, at least).

I am a bit dubious with the "check for equality" here because in theory property getters and setters can have all kinds of side effects (so eliding a get/set is not necessarily safe, even though it probably is in practice). (Not worth completely blocking the PR so feel free to ignore, but I don't really like it.)

On the other hand, unwrapping the end of the code (_internal_update tor reset the original values) as suggested by @QuLogic is probably a good idea (because at that point we know that all properties are indeed valid).

@anntzer anntzer mentioned this pull request Jan 29, 2026
Open
1 task
@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Feb 2, 2026

I'm on board with all of that @anntzer, updated. I'll move the text drawing change over to the other PR

anntzer
anntzer approved these changes Feb 2, 2026
View reviewed changes
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Maybe update the docstring to add "(and skips the check for duplicate aliases, for optimization)".

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Feb 2, 2026

Added

timhoffm
timhoffm reviewed Feb 2, 2026
View reviewed changes
scottshambaugh
scottshambaugh commented Feb 2, 2026
View reviewed changes
@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Feb 2, 2026

This should be squashed merged, it's a pretty messy commit history for what ended up being a very simple change.

@timhoffm timhoffm changed the title PERF: Add fast path to artist._cm_set PERF: Skip kwargs normalization in Artist._cm_set Feb 3, 2026
@timhoffm timhoffm merged commit 4e76477 into matplotlib:main Feb 3, 2026
35 of 37 checks passed
@timhoffm timhoffm added this to the v3.11.0 milestone Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic approved these changes

@anntzer anntzer anntzer approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

Performance

Projects

None yet

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

4 participants

@scottshambaugh @QuLogic @anntzer @timhoffm