-
Notifications
You must be signed in to change notification settings - Fork 51
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
Get the add-opcodes branch ready to merge upstream. #185
Comments
Our current stats show that the So, while there might be some benefit to this, it wouldn't be much. |
I have a branch laying around somewhere. Let me catch it up with main and benchmark to confirm your intuition. |
I assume that this isn't happening. We should probably implement |
Update: the benchmarking results are underwhelming. Branch here, for anyone curious. |
Sorry, the thing that Mark and I agreed on wasn't happening was a much older version of this idea (the branch is deleted). We might actually revive part of it if we get serious about reducing the code object size, since this would remove the need to have small ints in Regarding your branch, maybe the benchmark is slower because it has several conditional branches? I assume in the vast majority of cases where the code says |
Good observation! I'll re-run with just the int fast-paths. Maybe if we like the results, we could then explore adding proper specializations. |
A little better, but not much. |
The branch is here (a bit stale): https://github.com/gvanrossum/cpython/tree/add-opcodes
There's also this PR (also stale): python/cpython#25090
@brandtbucher The note said "convert to issue and assign to Brandt" so that's what I'm doing. But somehow I've forgotten context from our meeting -- do you recall? IIRC someone (@markshannon?) said "actually we should probably do that even though it didn't seem to move the benchmark numbers." But this version seems to only add an ADD_INT opcode, which seems redundant given your work on BINARY_OP. So maybe I'm just making a mess?
The text was updated successfully, but these errors were encountered: