Optimize getting config item values by saimn · Pull Request #17461 · astropy/astropy

saimn · 2024-11-26T20:49:34Z

Fix #12504 (I think, doing better might be possible but not sure without breaking some feature of our complicated configuration system):

As explained there, getting a config item is quite slow, values being stored within the ConfigObj object . And io.fits accesses config items when e.g. accessing the value of a card, this means a lot of config item calls for tables especially with many columns.

So here I'm adding a cache on the ConfigItem descriptor, allowing to get the key much faster while still storing its value on the ConfigObj object.

Description

This pull request is to address ...

Fixes #

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-11-26T20:50:03Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

saimn · 2024-11-26T20:50:46Z

Forgot to add some timings.
Before:

In [2]: %timeit fits.conf.enable_record_valued_keyword_cards
2.69 μs ± 502 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [3]: %timeit fits.getdata("astropy/io/fits/tests/data/tb.fits")
1.73 ms ± 338 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [2]: %timeit fits.getdata("astropy/io/fits/tests/data/memtest.fits")
11.5 ms ± 53.6 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

After:

In [2]: %timeit fits.conf.enable_record_valued_keyword_cards
55.2 ns ± 0.163 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [3]: %timeit fits.getdata("astropy/io/fits/tests/data/tb.fits")
1.09 ms ± 6.35 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [2]: %timeit fits.getdata("astropy/io/fits/tests/data/memtest.fits")
9.17 ms ± 11.2 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

pllim · 2024-11-26T20:55:29Z

+        try:
+            val = self.value
+        except AttributeError:
+            val = self.value = self()
+        return val


What about this?

Suggested change

try:

val = self.value

except AttributeError:

val = self.value = self()

return val

if not hasattr(self, "value"):

self.value = self()

return self.value

try/except has no cost if there is no exception, so I think it is good to use it (hasattr uses it internally, but now you do have the cost of the if statement). I'd try to shave off another few ns with

try: return self.value except AttributeError: self.value = val = self() return val

yeah try/except is commonly used in such cases both for perf and also hasattr can have some side effects so simpler is better here.

pllim · 2024-11-26T20:57:17Z

+        try:
+            del self.value
+        except AttributeError:
+            pass


Maybe like this instead? Though I don't see how this is equivalent to self.set(self.defaultvalue).

Suggested change

try:

del self.value

except AttributeError:

pass

if hasattr(self, "value"):

del self.value

For self.set(self.defaultvalue) it's annoying but I don't remember 😬 (I really should open PRs faster!)

note that the try/except form is more robust to parallelism (which might matter in the wake of free-threaded Python ?)

As long as hitting except is rare...

pllim · 2024-11-26T20:57:41Z

@@ -0,0 +1 @@
+Optimize getting config item values.


Can we roughly quantify this?

It's not a noticeable change if you get a few config values but it is for io.fits which uses config items when e.g. getting card values. So when reading many keywords or a table with many columns it starts to matter.

I didn't know FITS uses this... 🙈

mhvk

This looks good to me, though I haven't really looked at this code before, so it would be good for someone else to check too. Benchmarks certainly look good (the one bad one should be an unrelated fluke). But need some precommit fix or so.

mhvk

Nice, looks all good to me.

pllim

Thanks!

mhvk · 2025-05-13T09:54:28Z

Unfortunately, this caching of config items introduced a bug, in that reload_config does not reset the values cached here - see #18138.

This reverts commit 49765f4.

* Revert "Optimize getting config item values (#17461)" This reverts commit 49765f4. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

saimn added 3 commits November 26, 2024 21:32

remove metaclass

095da1c

optimize getting config item values

3d553ef

add changelog entry

076e164

saimn added config Performance labels Nov 26, 2024

saimn added this to the v7.1.0 milestone Nov 26, 2024

saimn requested a review from pllim November 26, 2024 20:53

pllim reviewed Nov 26, 2024

View reviewed changes

pllim added the benchmark Run benchmarks for a PR label Nov 26, 2024

mhvk approved these changes Nov 27, 2024

View reviewed changes

Comment thread astropy/config/configuration.py

apply suggestions + run pre-commit

118c113

mhvk approved these changes Nov 27, 2024

View reviewed changes

pllim approved these changes Dec 10, 2024

View reviewed changes

pllim merged commit 49765f4 into astropy:main Dec 10, 2024

saimn deleted the config branch December 10, 2024 08:13

This was referenced May 13, 2025

reload_config no longer properly reloads user-set values #18138

Closed

TST: doctest of conf.remote_timeout sometimes fails on double run #18135

Closed

pllim added a commit to pllim/astropy that referenced this pull request May 13, 2025

Revert "Optimize getting config item values (astropy#17461)"

07dbe5a

This reverts commit 49765f4.

pllim mentioned this pull request May 13, 2025

Revert PR 17461 (Optimize getting config item values) #18144

Merged

2 tasks

pllim mentioned this pull request May 14, 2025

ConfigNamespace is slow #12504

Open

Uh oh!

Conversation

saimn commented Nov 26, 2024

Description

Uh oh!

github-actions Bot commented Nov 26, 2024

Uh oh!

saimn commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

saimn commented Nov 26, 2024 •

edited

Loading