Repository navigation
Implement LOAD_GLOBAL opcode cache #72345
Description
| BPO | 28158 |
|---|---|
| Nosy | @brettcannon, @vstinner, @methane, @serhiy-storchaka, @1st1, @ztane |
| Files |
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 = 'https://github.com/1st1'
closed_at = <Date 2019-12-25.13:46:21.432>
created_at = <Date 2016-09-14.20:48:00.839>
labels = ['interpreter-core', '3.7', 'performance']
title = 'Implement LOAD_GLOBAL opcode cache'
updated_at = <Date 2019-12-25.13:46:21.425>
user = 'https://github.com/1st1'bugs.python.org fields:
activity = <Date 2019-12-25.13:46:21.425>
actor = 'methane'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2019-12-25.13:46:21.432>
closer = 'methane'
components = ['Interpreter Core']
creation = <Date 2016-09-14.20:48:00.839>
creator = 'yselivanov'
dependencies = []
files = ['44668']
hgrepos = []
issue_num = 28158
keywords = ['patch']
message_count = 14.0
messages = ['276485', '276519', '276520', '276521', '276522', '276645', '276647', '276697', '276703', '283616', '283629', '283662', '283681', '358865']
nosy_count = 6.0
nosy_names = ['brett.cannon', 'vstinner', 'methane', 'serhiy.storchaka', 'yselivanov', 'ztane']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue28158'
versions = ['Python 3.7']
Activity
1st1 commentedon Sep 14, 2016
The attached patch implements an opcode cache for LOAD_GLOBAL opcode, making it 2x faster.
The idea is to use the new co_extra field of code objects & PEP-509 to attach a cache structure pointing directly to the resolved global name. When globals or builtins are updated, the cache is invalidated.
The patch adds a new file "ceval_cache.h" which provides some infrastructure to ease the pain of implementing new caches for other opcode types. I can strip down all macros from that file if Victor finds it too hard to review.
Here's a simple script I used to make sure that the cache is working: https://gist.github.com/1st1/a9660aabdcf6b8bc6205b9fe39a82bba
You can also set OPCACHE_COLLECT_STATS to 1 to get debug stats output for the cache.
ztane commentedon Sep 15, 2016
I just noticed that you're actually testing a builtin instead of something in just module globals. How is the performance with module globals?
serhiy-storchaka commentedon Sep 15, 2016
Does this prolongate the lifetime of cached global objects?
1st1 commentedon Sep 15, 2016
No. I store borrowed references. The idea is that if the state of globals/builtins dict has changed, we invalidate the cache.
1st1 commentedon Sep 15, 2016
Serhiy, feel free to review the patch. Guido and Ned okayed it to be committed before 3.6b2.
Currently the patch only optimizes one opcode -- LOAD_GLOBAL, but cveal_cache.h file provides the infrastructure to easily implement more opcode caches. I can simplify that file (remove all macros) if it's too hard to review it.
1st1 commentedon Sep 16, 2016
I'm going to commit this patch tomorrow.
vstinner commentedon Sep 16, 2016
Can you please post results of the performance benchmark suite?
If you give me more time, I can review the patch.
serhiy-storchaka commentedon Sep 16, 2016
There is too much magic in ceval_cache.h. Since the patch adds caching only for the LOAD_GLOBAL opcode, it would much clearer if write functions only for the LOAD_GLOBAL opcode, without multilayer macros.
What will happen if there more than 255 LOAD_GLOBALs. Wouldn't 256th LOAD_GLOBAL return incorrect cached value for the the LOAD_GLOBAL (with index 0)?
ztane commentedon Sep 16, 2016
I wouldn't actually consider the builtin lookup speed almost at all, in all non-trivial applications the ratio of builtins to any other names will diminish; and if there is a tight loop, it is possible to always speed it up by
min = minif you do not need to support monkey patching.methane commentedon Dec 19, 2016
I'll update this patch and bpo-10401 and then run benchmark suite when I have time.
As far as I look quickly, bpo-10401 uses namei as cache key
and this patch uses opcode index as cache key, am I right?
1st1 commentedon Dec 19, 2016
Correct. Actually, if you don't mind, I'd like to update the patch myself. I have a few ideas how to restructure it and add support for LOAD_ATTR.
methane commentedon Dec 20, 2016
Sounds interesting.
I'll wait to touch this patch. There are enough time by Python 3.7
and enough other issues I can work.
methane commentedon Dec 20, 2016
memo: http://bugs.python.org/issue26219 may be relating to this.
methane commentedon Dec 25, 2019
This is implemented in bpo-26219.