Skip to content

Implement LOAD_GLOBAL opcode cache #72345

Closed
Closed
Implement LOAD_GLOBAL opcode cache#72345
Assignees
1st1
Labels
3.7 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

Description

@1st1
1st1
opened on Sep 14, 2016
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']

    Activity

    1. 1st1 commented on Sep 14, 2016

      @1st1
      1st1
      on Sep 14, 2016
      MemberAuthor

      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.

    2. 1st1
      self-assigned this
      on Sep 14, 2016
    3. 1st1
      added
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      performancePerformance or resource usage
      on Sep 14, 2016

    4. ztane commented on Sep 15, 2016

      @ztane
      ztanemannequin
      on Sep 15, 2016
      Mannequin

      I just noticed that you're actually testing a builtin instead of something in just module globals. How is the performance with module globals?

    5. serhiy-storchaka commented on Sep 15, 2016

      @serhiy-storchaka
      serhiy-storchaka
      on Sep 15, 2016
      Member

      Does this prolongate the lifetime of cached global objects?

    6. serhiy-storchaka
      added
      3.7 (EOL)end of life
      on Sep 15, 2016

    7. 1st1 commented on Sep 15, 2016

      @1st1
      1st1
      on Sep 15, 2016
      MemberAuthor

      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.

    8. 1st1 commented on Sep 15, 2016

      @1st1
      1st1
      on Sep 15, 2016
      MemberAuthor

      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.

    9. 1st1 commented on Sep 16, 2016

      @1st1
      1st1
      on Sep 16, 2016
      MemberAuthor

      I'm going to commit this patch tomorrow.

    10. vstinner commented on Sep 16, 2016

      @vstinner
      vstinner
      on Sep 16, 2016
      Member

      Can you please post results of the performance benchmark suite?

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

    11. serhiy-storchaka commented on Sep 16, 2016

      @serhiy-storchaka
      serhiy-storchaka
      on Sep 16, 2016
      Member

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

    12. ztane commented on Sep 16, 2016

      @ztane
      ztanemannequin
      on Sep 16, 2016
      Mannequin

      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.

    13. methane commented on Dec 19, 2016

      @methane
      methane
      on Dec 19, 2016
      Member

      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?

    14. 1st1 commented on Dec 19, 2016

      @1st1
      1st1
      on Dec 19, 2016
      MemberAuthor

      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.

    15. methane commented on Dec 20, 2016

      @methane
      methane
      on Dec 20, 2016
      Member

      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.

      Sounds interesting.
      I'll wait to touch this patch. There are enough time by Python 3.7
      and enough other issues I can work.

    16. methane commented on Dec 20, 2016

      @methane
      methane
      on Dec 20, 2016
      Member

      memo: http://bugs.python.org/issue26219 may be relating to this.

    17. methane commented on Dec 25, 2019

      @methane
      methane
      on Dec 25, 2019
      Member

      This is implemented in bpo-26219.

    18. methane
      closed this as completedon Dec 25, 2019
    19. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    Labels

    3.7 (EOL)end of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usage

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions