Implement LOAD_GLOBAL opcode cache

[1st1]

BPO	28158
Nosy	@brettcannon, @vstinner, @methane, @serhiy-storchaka, @1st1, @ztane
Files	load_globals_cache.patch

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

[1st1]

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]

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]

Does this prolongate the lifetime of cached global objects?

[1st1]

Does this prolongate the lifetime of cached global objects?

No. I store borrowed references. The idea is that if the state of globals/builtins dict has changed, we invalidate the cache.

[1st1]

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]

I'm going to commit this patch tomorrow.

[vstinner]

Can you please post results of the performance benchmark suite?

If you give me more time, I can review the patch.

[serhiy-storchaka]

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]

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 = min if you do not need to support monkey patching.

[methane]

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]

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?

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.