Skip to content

bpo-44501: pack call arguments on the compiler #26901

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

Closed
wants to merge 2 commits into from

Conversation

isidentical
Copy link
Member

@isidentical isidentical commented Jun 24, 2021

@pablogsal
Copy link
Member

pablogsal commented Jun 24, 2021

Hummm, unfortunately I think this is a lot of code for no general performance increase. Although it may make sense as a targeted optimization I don't see a clear win in terms of complexity and performance increase in the topical cases. I would like maybe other core Devs to mention what be they think, maybe we are collectively ok with it so we should merge it.

@isidentical
Copy link
Member Author

Hummm, unfortunately I think this is a lot of code for no general performance increase

I would argue that this is a lot of code, since it is just a single function in the AST optimizer (fold_call). No change on the compiler/interpreter, and the logic for the new function is pretty straight-forward (wrapping the arguments into a tuple). Also even though the pyperformance suite is unaffected by this (which was expected), there seem to be a lot of occurrences (nearly 100k cases, where this optimization can create a speed-up between %5 - %35 (maybe more?)), including some use cases within the standard library.

I would like maybe other core Devs to mention what be they think, maybe we are collectively ok with it so we should merge it.

We could wait for a bit on the tracker.

@pablogsal
Copy link
Member

here seem to be a lot of occurrences (nearly 100k cases, where this optimization can create a speed-up between %5 - %35 (maybe more?)), including some use cases within the standard library.

Right, but without a realistic benchmark to see the actual benefit of this is complicated to make an informed decision. For scoped optimizations we normally target higher numbers.

In any case, this is only my opinion on the matter but I also have an open mind about it 😉

@pablogsal
Copy link
Member

Actually, I retire my objection, as the core of the change is very scoped. I still think this is a small improvements but I think the changes may be worth the cost as they are very scoped and not very complex.

I'm +0.5 :)

@isidentical
Copy link
Member Author

@serhiy-storchaka what do you think? (I've posted the detailed answers to your question in the tracker)

@isidentical
Copy link
Member Author

@serhiy-storchaka if you have no objections (at least that is what I was able to infer from your last comment at the tracker), I intend to merge this.

@serhiy-storchaka
Copy link
Member

I afraid that we are in situation of having hammer and looking for nails. Too little code will get benefit from this optimization. It would be perhaps good if it only require adding 3 lines of code, but it adds 50.

@isidentical
Copy link
Member Author

Thanks for the reviews then! I still believe the absolute amount of code added (50~ lines) doesn't actually complicate the code, since it is in an isolated function in the AST optimizer, and shows significant improvements (up to %30 speed-up on some function calls, including re.match().group(...)) but I see your concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants