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.

Sign up for GitHub

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

Jump to bottom

PERF: Use fastpath for accessing option value in internals #49771

Merged
merged 3 commits into from
Dec 8, 2022
Merged

PERF: Use fastpath for accessing option value in internals #49771

merged 3 commits into from
Dec 8, 2022

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Nov 18, 2022 •
edited
Loading

Related to #49729

Accessing an option value is in general quite fast, but when it is in code that is called repeatedly in a for loop, it still adds up it seems.

Using the example from #49729:

import pandas as pd

def foo(df):
    for idx in df.index:
        df.at[idx, "bar"] = 3

df = pd.DataFrame(range(10000))
df["bar"] = 0

%timeit foo(df)
# 442 ms ± 16.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)  <-- main
# 316 ms ± 10.9 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)  <-- PR

I think this should be a safe way to access the option (but will add a test that ensures you can switch back and forth with CoW within a session).

Are we OK with this pattern?

@jorisvandenbossche jorisvandenbossche added this to the 1.5.3 milestone Nov 18, 2022
This was referenced Nov 18, 2022
Merged
Closed
jbrockmendel
jbrockmendel reviewed Nov 18, 2022
View reviewed changes
pandas/core/internals/managers.py
@@ -15,7 +15,7 @@

import numpy as np

from pandas._config import get_option
from pandas._config.config import _global_config
Copy link
Member

Choose a reason for hiding this comment

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

is this much faster than using pandas._config.config.options?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's in any case faster, since _global_config is just a plain python dict (which is quite fast), while the options object is a custom class mimicking attribute access:

In [32]: %timeit _global_config["mode"]["copy_on_write"]
52.6 ns ± 1.85 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [36]: %timeit pd._config.config.options.mode.copy_on_write
2.78 µs ± 22 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

Strangely it seems that this is even slower than the pd.get_option("mode.copy_on_write"):

In [37]: %timeit pd.get_option("mode.copy_on_write")
1.38 µs ± 14.6 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

Copy link
Member Author

@jorisvandenbossche jorisvandenbossche Nov 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we really want to avoid a private import, this is also an option:

In [39]: %timeit pd.options.d["mode"]["copy_on_write"]
85.9 ns ± 2.39 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

The options DictWrapper class seems to have a "public" d attribute exposing the underlying dict (_global_config)

Copy link
Member

Choose a reason for hiding this comment

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

not worth tying ourself in knots over, thanks for checking

@jbrockmendel
Copy link
Member

hmm at one point we had a code check that checked for private imports. guess that got lost somewhere along the way

@jorisvandenbossche jorisvandenbossche added the Performance Memory or execution speed performance label Nov 18, 2022
@jbrockmendel jbrockmendel mentioned this pull request Nov 21, 2022
Merged
@WillAyd
Copy link
Member

WillAyd commented Nov 21, 2022

Curious if you've looked at making get_option a class and using __slots__ instead for this? Seems like it would be the idiomatic approach to speeding up attribute access

@jorisvandenbossche
Copy link
Member Author

@WillAyd you mean the DictWrapper class that get_option/options uses? That is currently using a dynamic __getattr__ to look up attributes in a global dictionary that actually stores the option values.
What you suggest might be an option (but not directly an idea, didn't look into that), but that seems a bigger change in the internals of the options machinery. While what I am doing here is just using the fact that the current options internals is just a plain dictionary.

@jorisvandenbossche jorisvandenbossche marked this pull request as ready for review November 28, 2022 11:50
@jbrockmendel
Copy link
Member

jbrockmendel commented Nov 28, 2022 •
edited
Loading

Best Practices-wise, the best-case would be to try to optimize get_option. Next-best would be to make _global_config public-for-inernal-usage and canonize this as how we do option checks within pandas

@jorisvandenbossche
Copy link
Member Author

best-cast would be to try to optimize get_option

That would require a change of (or addition to) how get_option works, I think. Currently that takes a string (eg get_option("mode.copy_on_write")), and thus that requires parsing the string, and also has some overhead.

It's easy to rename _global_config to global_config (its module is already private) if that is preferred.

@jbrockmendel
Copy link
Member

yah even if we did optimize get_option a dict lookup will always be way faster

WillAyd
WillAyd approved these changes Nov 29, 2022
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm

@jorisvandenbossche jorisvandenbossche merged commit 0119bdc into pandas-dev:main Dec 8, 2022
@jorisvandenbossche jorisvandenbossche deleted the perf-cow-check-internals branch December 8, 2022 15:23
@meeseeksmachine meeseeksmachine mentioned this pull request Dec 8, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Dec 8, 2022
@jorisvandenbossche @meeseeksmachine
Backport PR pandas-dev#49771: PERF: Use fastpath for accessing option…
7d51e13 
… value in internals
@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 11, 2023
Merged
jorisvandenbossche added a commit that referenced this pull request Jan 13, 2023
@meeseeksmachine @jorisvandenbossche @phofl
Backport PR #49771 on branch 1.5.x (PERF: Use fastpath for accessing …
70a701d 
…option value in internals) (#50128)

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Patrick Hoefler <61934744+phofl@users.noreply.github.com>
@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd approved these changes

Assignees
No one assigned
Labels
Performance Memory or execution speed performance
Projects
None yet
Milestone
1.5.3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jorisvandenbossche @jbrockmendel @WillAyd