-
-
Notifications
You must be signed in to change notification settings - Fork 30k
-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Move functions to call objects into a new Objects/call.c file #73710
Comments
I propose to move functions to call objects into a new Objects/call.c file. It should easy maintainance since all moved functions are inter-dependent: it becomes easier to keep them consistent since they are in the same fle. I also have a small "hope" that moving "call" functions in the same file should help the compiler to produce more efficient code and that we will get a better "code placement". Closer functions should enhance the usage of the CPU instruction cache. I don't know exactly how to measure the effect of code placement. My plan is to push the change, and then watch speed.python.org timeline. Attached call.patch:
The change comes from the issue bpo-29465. Copy of my msg287257 from this issue:
I moved code from other .c files in Objects/, so for me it seems more Objects/call.c is 1500 lines long. IMHO the code became big enough to Once we have a call.c file, we may try other changes to enhance the code placement:
See also the issue bpo-29465: "Add _PyObject_FastCall() to reduce stack consumption. |
See also issue bpo-29502 "Should PyObject_Call() call the profiler on C functions, use C_TRACE() macro?": fixing this one should allow to remove fast-paths from ceval.c, since we now already have fast-paths in all "call" functions. |
I like the idea of moving all code related to calling objects in one one file. But this has a drawback. This breaks a history. This makes harder code exploration and merging. I would delay pushing this change until CPython repository be converted to Git. I heard Git better supports moving a code between files. And perhaps it might be easier to do this change by several commits -- separately moving a code from different files. |
Serhiy Storchaka added the comment:
Mercurial requires to use "hg cp file1.c file2.c" to keep file1 Git doesn't care. It is "smarter" (more convenient?). It uses an The migration to GitHub is ongoing, we are no more able to push to Any remark about call.c content itself? Does it look good to you? |
Oh, it's painful to have reviews and comments in two websites. Serhiy |
It is hard to make a review of changes of such kind. Common reviewing tools don't help with this. I would suggest you to make a series of commits that move a code from different files after migrating to Git. It may be easier to make a post-commit review. I said about Python/ because Object/ contains mainly implementations of concrete builtin types and Python/ contains some utility files (getargs.c, modsupport.c). But I think Objects/ is good place too. |
We have moved to GitHub and mandatory reviews with Pull Requests. So I created the PR #12 and removed the patch attached to this issue to avoid confusion. |
Benchmarks results. I don't know if the speedup is purely random, if I was just lucky, or if the change really makes Python faster... spectral_norm is a benchmark highly impacted by code placement. haypo@speed-python$ python3 -m perf compare_to /home/haypo/benchmarks/2017-02-10_00-20-default-e91ec62da088.json call_ref_e91ec62da088.json -G --min-speed=5
Faster (15):
Benchmark hidden because not significant (48): (...) |
commit c22bfaa
Thanks Serhiy and Naoki for your reviews! It's probably not perfect, but IMHO it's better than what we had previously, and we can rework this file later. About file history: "git blame Objects/call.c" shows me that the first and only author, *but* "git blame -C Objects/call.c" works as expected (show the full history)! |
New changeset c22bfaa by Victor Stinner in branch 'master': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: