Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
GH-98686: Quicken everything#98687
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 inadaptive_counter_start()
The counter is set to (2**backoff-1), there is no reason for this.
We could set the counter to3 or5 and the backoff to4.
@brandtbucher thoughts?
| /* The initial counter value is31 == 2**ADAPTIVE_BACKOFF_START - 1 */ | ||
| #define ADAPTIVE_BACKOFF_START5 | ||
| /* The initial counter value is1 == 2**ADAPTIVE_BACKOFF_START - 1 */ | ||
| #define ADAPTIVE_BACKOFF_START1 |
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 usedafter resetting to the adaptive form after repeated misses.
Maybe renameadaptive_counter_start toadaptive_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.
bedevere-bot commentedOct 26, 2022
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 |
bedevere-bot commentedNov 1, 2022
🤖 New build scheduled with the buildbot fleet by@brandtbucher for commit17292a3 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Uh oh!
There was an error while loading.Please reload this page.