bpo-26219: per opcode cache for LOAD_GLOBAL #12884
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch implements opcache only for LOAD_GLOBAL to ease review. Based on Yury's opcache3.patch.
methane
commented
Apr 22, 2019
methane
changed the title
pitrou
reviewed
May 10, 2019
| // TODO: LOAD_METHOD, LOAD_ATTR | ||
| if (opcode == LOAD_GLOBAL) { | ||
| co->co_opt_opcodemap[i] = ++opts; | ||
| if (opts > 254) { |
There was a problem hiding this comment.
Perhaps use 16-bit indices instead?
There was a problem hiding this comment.
To maximize "performance benefit / memory usage", I prefer using 1 byte for each instructions, instead of two bytes.
JimJJewett
reviewed
May 29, 2019
Include/code.h
Outdated
| // to cache object: co_opcache[co_opcache_map[next_instr/sizeof(_Py_CODEUNIT)]] | ||
| unsigned char *co_opcache_map; | ||
| _PyOpcache *co_opcache; | ||
| int32_t co_opcache_flag; // used to determine when create cache |
There was a problem hiding this comment.
(1) It looks like this is a usage count, rather than a flag. Maybe call it something like co_run_count?
(2) I don't think you need a full 32 bit int -- which means that it could fit into the same word as the following field. (and still leave room for some actual flags or something later, even on 32 bit)
There was a problem hiding this comment.
Yes -- currently. I don't think current algorithm to determine when enable cache
is stable, it can be reconsidered several times. So I don't think we need name represents
concrete usage of the variable for now.
JimJJewett
reviewed
May 29, 2019
| @@ -28,6 +28,9 @@ PyAPI_FUNC(void) _PyEval_SignalAsyncExc( | |||
| PyAPI_FUNC(void) _PyEval_ReInitThreads( | |||
| _PyRuntimeState *runtime); | |||
|
|
|||
| /* Private function */ | |||
| void _PyEval_Fini(void); | |||
There was a problem hiding this comment.
If it is really private, why include it in the header, instead of just putting the prototype in the .c file?
Also, why call call it a generic ...Eval_Fini as opposed to ...code_stats ?
There was a problem hiding this comment.
Because other private functions are put in "internal" headers too.
JimJJewett
reviewed
May 29, 2019
Include/internal/pycore_code.h
Outdated
| #endif | ||
|
|
||
| typedef struct { | ||
| PyObject *ptr; /* Cached pointer (stealed reference) */ |
There was a problem hiding this comment.
stolen reference? I'm more used to seeing that around something that can be called ... do you just mean that the object pointed to SHOULD not get an incref for this? (And is that to avoid circular references?)
There was a problem hiding this comment.
Ah, I was wrong. It is borrowed reference, not stolen.
JimJJewett
reviewed
May 29, 2019
| co->co_opcache_flag = 0; | ||
| co->co_opcache_size = 0; | ||
| co->co_opcache_map = NULL; | ||
| co->co_opcache = NULL; |
There was a problem hiding this comment.
Purely as a style question ... why did you change the order of the fields? (as opposed to initializing in definition order)
There was a problem hiding this comment.
I didn't changed order in here... I changed only order of struct for compactness and
readability. I didn't changed here because (a) I just missed it and (b) order in here is
not so important than order in struct.
|
|
DinoV
pushed a commit
to DinoV/cpython
that referenced
this pull request
Jan 14, 2020
This patch implements per opcode cache mechanism, and use it in only LOAD_GLOBAL opcode. Based on Yury's opcache3.patch in bpo-26219.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch implements opcache only for LOAD_GLOBAL to ease review.
Based on Yury's opcache3.patch.
https://bugs.python.org/issue26219