Skip to content

gh-146099: Optimize _GUARD_CODE_VERSION+IP via function version symbols#146101

Open
Fidget-Spinner wants to merge 5 commits intopython:mainfrom
Fidget-Spinner:opt_guard_code_version_2
Open

gh-146099: Optimize _GUARD_CODE_VERSION+IP via function version symbols#146101
Fidget-Spinner wants to merge 5 commits intopython:mainfrom
Fidget-Spinner:opt_guard_code_version_2

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Mar 18, 2026

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, the general approach looks sound.
I've spotted one correctness issue, and suggested a few minor tweaks

PyCodeObject *co = get_current_code_object(ctx);
if (co->co_version == version) {
_Py_BloomFilter_Add(dependencies, co);
if (sym_get_func_version(ctx->frame->callable) != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (sym_get_func_version(ctx->frame->callable) != 0) {
if (sym_get_func_version(ctx->frame->callable) == version) {

If the version is different this guard should fail

| KNOWN_CLASS--+ | | |
| | | | | | PREDICATE RECORDED_VALUE(known type)
| | | INT* | | | |
| | | | FUNC_VERSION | | | <- Anything below this level has a known truthiness.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says that anything below this line has known truthiness, and functions are always true.
Add a new line below the "Anything below this level has a known truthiness" line

printf("<v%u at %p>", sym->version.version, (void *)sym);
break;
case JIT_SYM_FUNC_VERSION_TAG:
printf("<fv%u at %p>", sym->func_version.func_version, (void *)sym);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
printf("<fv%u at %p>", sym->func_version.func_version, (void *)sym);
printf("<function version=%u>", sym->func_version.func_version);

Be a bit less cryptic (the pointer contains no useful information)

((PyFunctionObject *)sym->recorded_value.value)->func_version != version) {
sym_set_bottom(ctx, sym);
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}

@bedevere-app
Copy link

bedevere-app bot commented Mar 19, 2026

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@Fidget-Spinner
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Mar 19, 2026

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon March 19, 2026 14:40
@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Mar 19, 2026

I can confirm this fixes somewhat the regression seen on x86_64 for base16 benchmark in the past few days:

base16_large: Mean +- std dev: [base64_main] 10.2 ms +- 0.2 ms -> [base64_fixed] 9.38 ms +- 0.17 ms: 1.08x faster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants