-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-98686: Quicken everything #98687
GH-98686: Quicken everything #98687
Conversation
brandtbucher
commented
Oct 25, 2022
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Quicken everything #98686
1% faster:
|
I notice that startup (both with and without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original design made a distinction between the initial adaptive counter (which was zero as there was already a delay for quickening), and the first value for the backoff counter (15).
This PR loses that distinction.
Maybe we don't need that distinction, we already have to hit 53 misses before falling back to the adaptive form? Maybe we should retry specialization almost immediately and rely on the exponential backoff in case of repeated failures?
How about setting the initial value to 3 (or 5 or 7) to avoid specializing too aggressively at startup.
We can reuse the same value, as it makes little difference once misses are taken into account (53 + 7 is near enough 53 + 15)
One final point (not a fault of this PR, but relevant) is that in adaptive_counter_start()
The counter is set to (2**backoff-1), there is no reason for this.
We could set the counter to 3
or 5
and the backoff to 4
.
@brandtbucher thoughts?
/* The initial counter value is 31 == 2**ADAPTIVE_BACKOFF_START - 1 */ | ||
#define ADAPTIVE_BACKOFF_START 5 | ||
/* The initial counter value is 1 == 2**ADAPTIVE_BACKOFF_START - 1 */ | ||
#define ADAPTIVE_BACKOFF_START 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for backoff, after failure. Do you want to change this, or just lower the initial counter value?
uint8_t adaptive_opcode = _PyOpcode_Adaptive[opcode]; | ||
if (adaptive_opcode) { | ||
_Py_SET_OPCODE(instructions[i], adaptive_opcode); | ||
// Make sure the adaptive counter is zero: | ||
assert(instructions[i + 1] == 0); | ||
instructions[i + 1] = adaptive_counter_start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is what you want. adaptive_counter_start()
is supposed to be used after resetting to the adaptive form after repeated misses.
Maybe rename adaptive_counter_start
to adaptive_initial_backoff_value()
to avoid future confusion.?
What values did you try for the initial counter value?
1 seems low, as we might be spending too much effort specializing startup code.
When you're done making the requested changes, leave the comment: |
Recapping our discussion on Monday: An initial value of It's not strictly necessary to use the same value for the initial warmup delay and the initial backoff value, but |
🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit 17292a3 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |