Repository navigation
[rocjitsu] Allocate decoder pools on demand - #13074
Merged
Merged
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
sjain-stanford
left a comment
sjain-stanford
left a comment
Member
There was a problem hiding this comment.
I think it would help to tighten the new regressions in emulation/rocjitsu/tests/decoder_c_api_test.cpp:
- At
decoder_c_api_test.cpp:159, useEXPECT_NEinstead ofASSERT_NE, or restore the pool on every exit. A fatal assertion returns whilefirstis still pool-backed and the pool is disabled, sending its storage to global delete. A standalone ASan probe reproduces a double-free during decoder teardown.pool->owns(nullptr)is safe, so the nonfatal check can continue to the existing re-enablement. - At
decoder_c_api_test.cpp:171, add non-null allocator hooks withalloc_pool_ == nullptr, then disable and destroy an unused decoder and check that the hooks survive. Both existing scenarios still pass with an unguardedinvalidate_allocator_pool(nullptr); the null-context case distinguishes the new guard. This covers manually installed hooks beyond ordinary decoder pools. - At
decoder_c_api_test.cpp:18, use a small layout budget for bothDecoderand a representativeIsaDecoder. The current comparison against the 65,552-byte pool allows a 32 KiB embedded buffer to pass. The measured Linux x86-64 sizes are 16 and 32 bytes; any exact assertions should be scoped to the supported ABI.
kuhar
force-pushed
the
users/kuhar/lazy-decoder-pool
branch
from
October 8, 2026 03:05
d8f5ee9 to
92cef83
Compare
sjain-stanford
approved these changes
Oct 8, 2026
sjain-stanford
left a comment
sjain-stanford
left a comment
Member
There was a problem hiding this comment.
LGTM apart from one comment.
Allocate the instruction pool on first enablement instead of embedding its 64 KiB backing in every decoder. Production decoders use heap storage and never enable the pool. On Linux x86-64, shrink Decoder from 65,568 to 16 bytes and the CDNA5 decoder from 65,584 to 32 bytes, saving 16 MiB for 256 CUs. Retain the pool across disable/enable cycles and invalidate active and suppressed TLS references before destruction. Keep pool ownership private and document that pooled instructions must be deleted with their pool active. Guard the compact layout and verify that construction and decoding leave the pool absent until first enablement. Cover retained pool ownership, heap allocation while disabled, repeated enablement, and preservation of unrelated allocator state. Validate with 589 focused tests after rebase. Confirm that the allocation regression rejects an eager-pool factory mutation. Earlier qualification includes 74 smoke runs across five GPU targets. Matching Clang 23 Release builds reduce median peak RSS by 18.7 MiB for gfx1250 Gluon and 23.9 MiB for gfx950 IREE over three runs each. Runtime changes remain workload dependent.
kuhar
force-pushed
the
users/kuhar/lazy-decoder-pool
branch
from
October 8, 2026 14:20
92cef83 to
962a758
Compare
newling
approved these changes
Oct 8, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
anujshuk-amd
pushed a commit
that referenced
this pull request
Oct 12, 2026
Allocate the instruction pool on first enablement instead of embedding its 64 KiB backing in every decoder. Production decoders use heap storage and never enable the pool. On Linux x86-64, shrink Decoder from 65,568 to 16 bytes and the CDNA5 decoder from 65,584 to 32 bytes, saving 16 MiB for 256 CUs. Retain the pool across disable/enable cycles and invalidate active and suppressed TLS references before destruction. Keep pool ownership private and document that pooled instructions must be deleted with their pool active. Guard the compact layout and cover heap allocation while disabled, repeated enablement, and preservation of unrelated allocator state. Validate with 529 focused tests and 74 smoke runs across five GPU targets. Matching Clang 23 Release builds reduce median peak RSS by 18.7 MiB for gfx1250 Gluon and 23.9 MiB for gfx950 IREE over three runs each. Runtime changes remain workload dependent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Allocate the instruction pool on first enablement instead of embedding its 64 KiB backing in every decoder. Production decoders use heap storage and never enable the pool. On Linux x86-64, shrink Decoder from 65,568 to 16 bytes and the CDNA5 decoder from 65,584 to 32 bytes, saving 16 MiB for 256 CUs.
Retain the pool across disable/enable cycles and invalidate active and suppressed TLS references before destruction. Keep pool ownership private and document that pooled instructions must be deleted with their pool active. Guard the compact layout and cover heap allocation while disabled, repeated enablement, and preservation of unrelated allocator state.
Validate with 529 focused tests and 74 smoke runs across five GPU targets. Matching Clang 23 Release builds reduce median peak RSS by 18.7 MiB for gfx1250 Gluon and 23.9 MiB for gfx950 IREE over three runs each. Runtime changes remain workload dependent.