-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-126703: add freelist for PyComplexObject's #135233
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
Conversation
@skirpichev Would you like to provide benchmark enhancement for this change? |
Oops, I just forgot to post one. Description updated. |
Please share the pyperformance benchmark results. |
Well, it shouldn't be affected. This collection has no benchmarks for complex math at all:
Probably less than floats, but not too much. |
AFAICS, real-world workloads that don't use complex numbers will pay the price of:
In applications that do use complex numbers, is it likely that they need to create/destroy them quickly? To review this I'll need to teach myself about Python's freelists; I can do that next week. |
I believe that we use pyperformance benchmark as the real world proxy. So if there is no benchmark for it, I am unsure. At least someone needs to share the benchmark for this. If not, people will add new PR for other built-in types with a microbenchmark, which would not be worth adding. We need criteria for this. |
Yes, just a new empty
That happens in every arithmetic operation. Here is a benchmark with a simple sin() implementation:
# bench2.py
def mysin(z):
i, lasts, s, fact, num, sign = 1, 0, z, 1, z, 1
while s != lasts:
lasts = s
i += 2
fact *= i*(i-1)
num *= z*z
sign *= -1
s += num/fact*sign
return s
import pyperf
runner = pyperf.Runner()
runner.bench_func('mysin(1j)', mysin, 1j)
runner.bench_func('mysin(1+1j)', mysin, 1+1j)
You are not alone, this is new for me as well :D IIUC (see help topic), this technique is not something, allowed for external C extensions.
I doubt we have too much built-in types. Being a built-in type is a sign from the real world too ;-) |
I see this as a PR that speeds up complex numbers, not “X% of Python's real-world usage”. And that's fine.
Why shouldn't the criteria be “any small builtin type where typical usage needs many instances”?
Why not? What would be the downside of all such built-in types using freelists? |
Then why don’t we just register all built-in objects to the freelist? I think our primary focus is on keeping maintenance costs low. Once we add complex to the freelist, we’ll need to maintain it anyway. We already have over 20 object types registered. If we’re going to add more, shouldn’t we also consider a more manageable way to maintain them? We take a similar approach when deciding not to add a fast path, if the benefit isn’t significant enough, we skip it. From what I’ve observed, most recent optimizations are based on pyperformance results. That said, if you still think we should add this, please go ahead. I just wanted to share my concern about adding it by default. Once an object is added to the freelist, it will usually see some benefit, but we should still weigh that against the maintenance burden. |
We already have an API for that, the |
I thought of managing the list of types and generating the |
Oh ok, I see. I'm not sure that it's worth it. This list evolves rarely and IMO the maintenance cost is low. |
But pyperformance can't measure any performance changes in the Python core type, complex. Is that a bug or a feature? |
In the past, we observed many optimization enhancements for the binary operation, especially for the int or float types. |
This is possible for complex type as well, but it's another story. |
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.
LGTM. The change is short and 1.09x faster on the mysin() micro-benchmark is worth it IMO.
@corona10: You consider that the change is not worth it?
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.
LGTM, Okay, let's consider the possibility of usage in the scientific area.
Micro-benchmark: