-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
PERF: Improve efficiency of BlockValuesRefs
#59598
Conversation
@Tolker-KU - can you run and post ASV results |
BlockValuesRefs
BlockValuesRefs
Thanks for taking a look a this. I've tried hard to get meaningful runs of the asv benchmarks but I think my computer is so old and slow that noise is dominating the results. I'm getting weird results where some benchmarks are much faster and some are much slower To show the gains from this I've run below micro-benchmark >>> N = 10_000
>>> arr = np.random.normal(size=(50, N))
>>> df = pd.DataFrame(arr)
>>> %timeit -n10000 list(df.iterrows())
2.43 ms ± 268 μs per loop (mean ± std. dev. of 7 runs, 10,000 loops each) <- before
2.1 ms ± 117 μs per loop (mean ± std. dev. of 7 runs, 10,000 loops each) <- after |
@@ -890,12 +890,12 @@ cdef class BlockValuesRefs: | |||
|
|||
def __cinit__(self, blk: Block | None = None) -> None: | |||
if blk is not None: | |||
self.referenced_blocks = [weakref.ref(blk)] | |||
self.referenced_blocks = [PyWeakref_NewRef(blk, None)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this actually change anything? I am under the impression that Cython would be smart enough to handle this the same in both cases.
Instead of looking at the benchmarks (which can be difficult to run and flaky), you can also inspect the Cython annotations before/after the change. Are they actually different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Glad you brought that up. I should've been more explicit in the description, as the motivation for this PR is that the code-gen changes quite dramatically. An example from one of the methods below. Note the code generated for the method does not fit on one screen before the change
Before
After
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for posting. That's...quite the difference
Thanks @Tolker-KU |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Improve efficiency of
BlockValuesRefs
by allowing for more efficient C code-gen