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.
Uh oh!
There was an error while loading. Please reload this page.
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.
win_direct_tlsenableMI_WIN_DIRECT_TLSMI_WIN_DIRECT_TLS- Use only direct TLS slots on Windows to avoid extra tests in the malloc fast path (only works if the program uses less than 64 TlsAlloc'd slots in total)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.
Can you say more about the motivation for enabling this option and why you think it's correct? I'm not familiar with TLS but codex tells me that the number of TLS can be environment specific, meaning, it can be out of Ruff's control.
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.
mimallocmakes only two calls toTlsAlloc():_mi_theap_default_slotand_mi_theap_cached_slot, however, what matters here isn't the calls themselves, but which indices have already been occupied by the process prior tomimallocIn other words,
mimalloccalls must fall within the range0..63.The first 64 slots are fast because they are physically embedded directly into the TEB (Thread Environment Block) structure at a fixed offset, and accessing them requires only a single read instruction via a segment register (gs/fs), without any additional pointer dereferencing.
Windows guarantees a minimum of 64 TLS indices per process, and modern versions of Windows can have up to 1,088.
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.
microsoft/mimalloc#1359
Maybe we shouldn't enable this feature, since it's random
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'd prefer not to enable it, unless benchmark differences are significant. If they are, we should then probably dig a little deeper on when it's okay to enable this optimization