Repository navigation
Move functions to call objects into a new Objects/call.c file #73710
Description
| BPO | 29524 |
|---|---|
| Nosy | @vstinner, @methane, @serhiy-storchaka |
| PRs |
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:
assignee = None
closed_at = <Date 2017-02-12.22:03:48.773>
created_at = <Date 2017-02-10.12:42:39.194>
labels = ['interpreter-core', '3.7']
title = 'Move functions to call objects into a new Objects/call.c file'
updated_at = <Date 2017-03-25.00:25:55.303>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2017-03-25.00:25:55.303>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-12.22:03:48.773>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2017-02-10.12:42:39.194>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 29524
keywords = ['patch']
message_count = 10.0
messages = ['287527', '287528', '287545', '287555', '287556', '287559', '287582', '287588', '287657', '290445']
nosy_count = 3.0
nosy_names = ['vstinner', 'methane', 'serhiy.storchaka']
pr_nums = ['12']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue29524'
versions = ['Python 3.7']
Activity
vstinner commentedon Feb 10, 2017
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:
---
Serhiy Storchaka added the comment:
I moved code from other .c files in Objects/, so for me it seems more
natural to add the code in Objects/ as well. It seems like most of the
code in Python/ is not "aware" of types defined in Objects/. But I
don't have a strong opinion on the right directory.
Objects/call.c is 1500 lines long. IMHO the code became big enough to
justify to move it to a new file ;-)
---
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.
vstinner commentedon Feb 10, 2017
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.
serhiy-storchaka commentedon Feb 10, 2017
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.
vstinner commentedon Feb 10, 2017
Serhiy Storchaka added the comment:
Mercurial requires to use "hg cp file1.c file2.c" to keep file1
history in file2.
Git doesn't care. It is "smarter" (more convenient?). It uses an
heuristic to automatically detect when code is moved between files.
The migration to GitHub is ongoing, we are no more able to push to
Mercurial anyway :-D
Any remark about call.c content itself? Does it look good to you?
vstinner commentedon Feb 10, 2017
Oh, it's painful to have reviews and comments in two websites. Serhiy
left a comment on the review in fact:
"Should be added also in PCbuild/pythoncore.vcxproj.filters."
serhiy-storchaka commentedon Feb 10, 2017
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.
vstinner commentedon Feb 11, 2017
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.
vstinner commentedon Feb 11, 2017
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
Slower (1):
Faster (15):
Benchmark hidden because not significant (48): (...)
vstinner commentedon Feb 12, 2017
commit c22bfaa
Author: Victor Stinner <victor.stinner@gmail.com>
Date: Sun Feb 12 19:27:05 2017 +0100
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)!
vstinner commentedon Mar 25, 2017
New changeset c22bfaa by Victor Stinner in branch 'master':
bpo-29524: Add Objects/call.c file (#12)
c22bfaa