classification
Title: Move functions to call objects into a new Objects/call.c file
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, inada.naoki, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2017-02-10 12:42 by haypo, last changed 2017-03-25 00:25 by haypo. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12 haypo, 2017-02-11 00:47
Messages (10)
msg287527 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 12:42
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 #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 #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 #29465: "Add _PyObject_FastCall() to reduce stack consumption.
msg287528 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 12:47
See also issue #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.
msg287545 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-10 15:04
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.
msg287555 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 17:13
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?
msg287556 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-10 17:14
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."
msg287559 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-10 17:50
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.
msg287582 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-11 00:48
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.
msg287588 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-11 02:14
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): (...)
msg287657 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-02-12 22:03
commit c22bfaae83ab5436d008ac0d13e7b47cbe776f08
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Sun Feb 12 19:27:05 2017 +0100

    bpo-29524: Add Objects/call.c file (#12)
    
    * 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.

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)!
msg290445 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-03-25 00:25
New changeset c22bfaae83ab5436d008ac0d13e7b47cbe776f08 by Victor Stinner in branch 'master':
bpo-29524: Add Objects/call.c file (#12)
https://github.com/python/cpython/commit/c22bfaae83ab5436d008ac0d13e7b47cbe776f08
History
Date User Action Args
2017-03-25 00:25:55hayposetmessages: + msg290445
2017-02-12 22:03:48hayposetstatus: open -> closed
resolution: fixed
messages: + msg287657

stage: resolved
2017-02-11 02:14:45hayposetmessages: + msg287588
2017-02-11 00:48:33hayposetmessages: + msg287582
2017-02-11 00:47:45hayposetfiles: - call.patch
2017-02-11 00:47:37hayposetpull_requests: + pull_request28
2017-02-10 17:50:30serhiy.storchakasetmessages: + msg287559
2017-02-10 17:14:32hayposetmessages: + msg287556
2017-02-10 17:13:32hayposetmessages: + msg287555
2017-02-10 15:04:19serhiy.storchakasetmessages: + msg287545
2017-02-10 12:47:15hayposetmessages: + msg287528
2017-02-10 12:42:39haypocreate