Read an indexed object's record fields in one place - #2962
Open
pyricau wants to merge 1 commit into
Open
Conversation
Three functions built an IndexedInstance, an IndexedObjectArray and an IndexedPrimitiveArray from a ByteSubArray, each with its own copy of which field is read in which order and how many bytes it takes: objectAtIndex, indexedObjectOrNull, and the per type sequences. Classes were already read through a single ByteSubArray.readClass(), so this gives the other three record types the same treatment. Three copies is what made storing an entry's class as an index rather than as an id a six line change in the commit before this one, and the round trip test in HprofIndexParsingTest exists because one of those copies had the wrong index arithmetic, which it only covers for one of the four types. Both callers keep the order they read in: objectAtIndex walks the four index ranges, and indexedObjectOrNull reads from the map it just searched, classes first, since resolving an instance's class is most of what it is asked for. objectIdIsIndexed asked each of the four maps for a value it threw away, allocating a ByteSubArray per map to do it, where the index it wants is what objectIndexOrMinusOne already returns. An out of range index passed to objectAtIndex now reports the range it should have been in, instead of failing a bare require on a shifted index three branches later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pyricau
force-pushed
the
index-lookup-speed
branch
from
August 21, 2026 15:41
3b665da to
7e89880
Compare
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.
This started as an attempt to make object id lookups faster, and ends up as a cleanup, because the thing that was fast enough to matter isn't worth what it costs. The measurements are at the bottom; the diff is the part I'd keep either way.
What's in the diff
Three functions each built an
IndexedInstance, anIndexedObjectArrayand anIndexedPrimitiveArrayfrom aByteSubArray, with their own copy of which field is read in which order and how many bytes it takes:objectAtIndex,indexedObjectOrNull, and the per typeindexed*Sequence()functions. Classes were already read through a singleByteSubArray.readClass(), so the other three record types now get the same treatment.Three copies is why storing an entry's class as an index rather than as an id (#2964, the commit right before this one) had to change six call sites, and the round trip test in
HprofIndexParsingTestexists because one of those copies had the wrong index arithmetic — it only covers one of the four types.Both callers keep the read order they had:
objectAtIndexwalks the four index ranges,indexedObjectOrNullreads from the map it just searched, classes first.Also in here:
objectIdIsIndexedasked each of the four maps for a value it threw away, allocating aByteSubArrayper map to do it, where the index it wants is whatobjectIndexOrMinusOnealready returns.objectAtIndexnow reports the range it should have been in, instead of failing a barerequireon a shifted index three branches later.indexedObjectOrNullgets a KDoc saying why it searches classes first whereobjectIndexOrMinusOnesearches them last.No behavior change, no format change, no public API change.
What I measured and did not keep
A cache in front of the four index searches.
object id => objectIndex, 1024Intslots, 4 KB, verified on read rather than trusted, so it needed no synchronization. It worked: 9313 ms → 8715 ms (1.07x) for an analysis plus a dominator tree build on a 172 MB Android heap dump of 1713828 objects, with reads per lookup going 18.19 → 5.89 and searches 45809397 → 11013684, at an 85.4% hit rate. Same 1.07x on the 25 MB dump in the test resources. Both signature checked against the unmodified implementation.Not landing it, because it stops
HprofInMemoryIndexbeing trivially thread safe. It is safe — anIntwrite can't tear, every value stored is a real index, and it's checked against the id at that index before use — but "safe" would rest on a paragraph of reasoning where it currently rests on the class being read only, and any later edit inside the class could quietly break it. That's a bad trade for 7% in a class four parallel readers depend on.Where the 7% comes from is worth writing down: an instance's class is looked up once per instance of that class, so 81% of the searches an analysis performs are of the class index (37.7M of 46.3M, 568M of 728M probes). Somewhere thread confined — a traversal, say — that locality is still there for the taking, without touching the index.
Interpolation search over the sorted id arrays. Bounded at 3 interpolated probes before handing off to binary search, verified to return exactly what binary search returns (including insertion points on misses) for all 46M+ real keys captured from an analysis. It is slower everywhere, 0.37x to 0.58x per search, because it takes more probes: within one record type, ART heap spaces leave gaps up to 1066591x the mean gap, so one interpolated probe lands 11.54% to 49.78% of the array from the answer on average against the 25% binary search gets by construction. Structural, not a tuning problem — the idea comes from an analyzer whose offset arrays are gigabytes, where Shark's per type arrays are 1 to 15 MB and largely cache resident.
Testing
./gradlew buildpasses, includingHprofRetainedHeapPerfTestandHprofIOPerfTest, which freeze retained bytes and bytes read.🤖 Generated with Claude Code