| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks mostly good, just 2 comments that also explain the CI failure.
Sorry, something went wrong.
|
Thanks for the review! I'll continue working on it tomorrow (Monday) 🙂 |
Sorry, something went wrong.
|
I just merged #135761 in. Sorry but you'll have to merge the changes in, as I did not ultimately add _SWAP_FAST. |
Sorry, something went wrong.
There was a problem hiding this comment.
Yay, thank you!
Sorry, something went wrong.
|
I will do some microbenchmarking to see if this does anything. Give me a sec. |
Sorry, something went wrong.
|
On this script: import pyperf
def func():
x = 1
for _ in range(120000000):
type(x)
if __name__ == "__main__":
runner = pyperf.Runner()
runner.bench_func('micro', func)
Mean +- std dev: [no-opt] 918 ms +- 17 ms -> [opt] 897 ms +- 21 ms: 1.02x faster
So it's slightly faster. However, note that this optimization won't fully pay off till we get #135465 merged. So let's wait for now till then. Thanks! Basically right now there's the extra cost of reading and writing to the stack due to the extra _POP_TOP. We're eliminating 1 reference count check in return. This seems to be barely worth it, but I crunched the numbers before and once the stack becomes registers, it becomes very worth it. If you're interested, I benchmarked my own branch with this (higher is better): CPython base JIT: Number of long_calcul per second: 10.23 ± 0.13 CPython TOS caching: Number of long_calcul per second: 9.60 ± 0.37 CPython decref binary op int elimination: Number of long_calcul per second: 10.14 ± 0.07 CPython TOS + decref binary op int elimination: Number of long_calcul per second: 11.45 ± 0.10 So we should expect to see an around ~10% speedup once TOS caching is merged, and we start doing this! |
Sorry, something went wrong.
|
@tomasr8 congrats!!! I decreased the loop count, rebased your PR on top of Mark's TOS caching PR, and did a micro bench with the script above. TOS Caching vs TOS Caching + This PR Mean +- std dev: [no-opt] 210 ms +- 3 ms -> [opt] 195 ms +- 1 ms: 1.08x faster So 8% improvement if we have TOS caching enabled on type operations that load a local variable! |
Sorry, something went wrong.
|
Awesome stuff! And a pretty nice speedup for a relatively simple optimization too! |
Sorry, something went wrong.
| } | ||
|
|
||
| tier2 op(_SWAP_CALL_ONE_LOAD_CONST_INLINE_BORROW, (ptr/4, callable, null, arg -- value, a)) { | ||
| PyStackRef_CLOSE(arg); |
There was a problem hiding this comment.
This shouldn't be here, right? Because we're leaving arg on the stack in a?
Sorry, something went wrong.
There was a problem hiding this comment.
Veery late response 😅 but I changed and renamed this op to _SHUFFLE_2_LOAD_CONST_INLINE_BORROW since it's analogous to the recently added _SHUFFLE_3_LOAD_CONST_INLINE_BORROW. This also resolves the issue with closing the ref! :)
Sorry, something went wrong.
|
I think it should be good now! Though I haven't yet caught up on the latest changes in the JIT so another pair of eyes would be nice :) |
Sorry, something went wrong.
There was a problem hiding this comment.
Just one change, otherwise LGTM!
Sorry, something went wrong.
|
Windows failure is unrelated. Probably due to the other known bug in executor management. I'm merging this, and will be responsible for any breakages/reverts. Thank Tomas, and happy holidays! |
Sorry, something went wrong.
|
Thanks for the review, Ken! Happy holidays to you as well! :) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Depends on #135761