Move functions to call objects into a new Objects/call.c file

[vstinner]

BPO	29524
Nosy	@vstinner, @methane, @serhiy-storchaka
PRs	bpo-29524: Add Objects/call.c file #12

^{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']

[vstinner]

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:

• Add Objects/call.c file
• Move all functions to call objects in a new Objects/call.c file.
• Rename fast_function() to _PyFunction_FastCallKeywords().
• Copy null_error() from Objects/abstract.c
• Inline type_error() in call.c to not have to copy it, it was only called once.
• Export _PyEval_EvalCodeWithName() since it is now called from call.c.

The change comes from the issue bpo-29465. Copy of my msg287257 from this issue:
---
Serhiy Storchaka added the comment:

Isn't the Python directory more appropriate place for call.c?

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:

• Add "#define PY_LOCAL_AGGRESSIVE" (only used by Visual Studio)
• Tag functions with _Py_HOT_FUNCTION: see the issue bpo-28618 for discussion on __attribute__((hot)). The last time I checked, it wasn't enough to fix all "code placement issues".
• Reorder functions in call.c: I dislike this idea, IMHO it's too hard to guess ourself what is the best order, and it's likely to produce many commits for little benefit

See also the issue bpo-29465: "Add _PyObject_FastCall() to reduce stack consumption.

[vstinner]

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]

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]

Serhiy Storchaka added the comment:

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.

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]

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]

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]

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]

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):

• spectral_norm: 242 ms +- 3 ms -> 282 ms +- 2 ms: 1.16x slower (+16%)

Faster (15):

• xml_etree_process: 193 ms +- 4 ms -> 173 ms +- 3 ms: 1.11x faster (-10%)
• call_method: 12.4 ms +- 0.6 ms -> 11.3 ms +- 0.3 ms: 1.10x faster (-9%)
• hexiom: 18.5 ms +- 0.2 ms -> 17.1 ms +- 0.1 ms: 1.09x faster (-8%)
• sympy_expand: 940 ms +- 12 ms -> 866 ms +- 13 ms: 1.09x faster (-8%)
• regex_effbot: 5.39 ms +- 0.09 ms -> 4.99 ms +- 0.06 ms: 1.08x faster (-7%)
• chaos: 235 ms +- 2 ms -> 219 ms +- 2 ms: 1.07x faster (-7%)
• unpickle_pure_python: 686 us +- 11 us -> 639 us +- 7 us: 1.07x faster (-7%)
• chameleon: 22.6 ms +- 0.4 ms -> 21.0 ms +- 0.3 ms: 1.07x faster (-7%)
• nqueens: 220 ms +- 2 ms -> 205 ms +- 3 ms: 1.07x faster (-7%)
• telco: 15.0 ms +- 0.6 ms -> 14.0 ms +- 0.2 ms: 1.07x faster (-6%)
• sympy_str: 423 ms +- 4 ms -> 399 ms +- 3 ms: 1.06x faster (-6%)
• scimark_monte_carlo: 223 ms +- 9 ms -> 211 ms +- 8 ms: 1.06x faster (-6%)
• xml_etree_generate: 223 ms +- 3 ms -> 210 ms +- 2 ms: 1.06x faster (-6%)
• xml_etree_iterparse: 192 ms +- 3 ms -> 182 ms +- 4 ms: 1.05x faster (-5%)
• sympy_sum: 191 ms +- 7 ms -> 181 ms +- 7 ms: 1.05x faster (-5%)

Benchmark hidden because not significant (48): (...)

[vstinner]

commit c22bfaa
Author: Victor Stinner <victor.stinner@gmail.com>
Date: Sun Feb 12 19:27:05 2017 +0100

bpo-29524: Add [Objects/call.c](https://github.com/python/cpython/blob/main/Objects/call.c) file (#12)

* Move all functions to call objects in a new [Objects/call.c](https://github.com/python/cpython/blob/main/Objects/call.c) file.
* Rename fast_function() to _PyFunction_FastCallKeywords().
* Copy null_error() from [Objects/abstract.c](https://github.com/python/cpython/blob/main/Objects/abstract.c)
* Inline type_error() in call.c to not have to copy it, it was only
  called once.
* Export _PyEval_EvalCodeWithName() since it is now called
  from call.c.

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]

New changeset c22bfaa by Victor Stinner in branch 'master':
bpo-29524: Add Objects/call.c file (#12)
c22bfaa