Skip to content

Conversation

BobTheBuidler
Copy link
Contributor

@BobTheBuidler BobTheBuidler commented Oct 22, 2025

This doesn't do much, currently it enables just one optimization which already exists in the codebase: it helps mypyc determine whether to use CPyTagged_XDECREF or CPyTagged_XDecRef within a particular block

But it's semantically reasonable to accept a rare flag for any boolean branch, even if it doesn't do too much optimizing, and it opens the door for us to implement further optimizations in this area

@BobTheBuidler BobTheBuidler marked this pull request as ready for review October 22, 2025 01:23
@A5rocks
Copy link
Collaborator

A5rocks commented Oct 22, 2025

I imagine it would be nicer if we bundled a change like this with optimization (mark branches with pragmas). Otherwise I feel like complexity doesn't automatically help! Also that would let the actual perf impacts be looked at (branch misprediction counts?).

@BobTheBuidler
Copy link
Contributor Author

BobTheBuidler commented Oct 22, 2025

Well, it exists for this one case so it seems someone else decided this case was worthy enough to write a bunch of code for. I'm not the one who wrote that optimization, I'm just writing a tiny bit more code so other parts of the codebase can reuse it

@A5rocks
Copy link
Collaborator

A5rocks commented Oct 22, 2025

Ah I see, I missed that.

@BobTheBuidler
Copy link
Contributor Author

BobTheBuidler commented Oct 22, 2025

@A5rocks originally I was trying to figure out how to use this rare kwarg to wrap C-level if-checks with likely and unlikely, which provides additional branch context to the C compiler, which may or may not allow the compiler to optimize some more stuff internally

I failed at that for the time being but that can still come down the pipe after this once I have more time to experiment with it

builder.read(instance), builder.none_object(), "is", line
)
builder.add_bool_branch(comparison, class_block, instance_block)
builder.add_bool_branch(comparison, class_block, instance_block, rare=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one might be questionable

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants