Reduce per-field hashing and iteration on the composite/Parquet indexing hot path - #22686
Draft
Bukhtawar wants to merge 4 commits into
Draft
Reduce per-field hashing and iteration on the composite/Parquet indexing hot path#22686Bukhtawar wants to merge 4 commits into
Bukhtawar wants to merge 4 commits into
Conversation
Contributor
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 21485c5. ⛔ Hard block: Issues at Medium severity or above will block this PR from merging.
The table above displays the top 10 most important findings. Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
…ngest ParquetDocumentInput.addField deduped single-valued fields via an IdentityHashMap-backed set, so every field per document paid System.identityHashCode (JVM_IHashCode — a top frame in bulk-indexing CPU profiles). MappedFieldType inherits identity hashCode; the mapping layer forbids two mappers sharing a name, so a HashSet keyed on field name is equivalent and uses String's cached hash instead. O(n), no quadratic scan. Adds a realistic JMH benchmark exercising the real addField over genuine MappedFieldType instances, wired via a jmh source set in the plugin. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Bukhtawar
force-pushed
the
parquet-indexing-hotspot-fixes
branch
from
August 8, 2026 23:46
655c923 to
ed4fff8
Compare
CompositeDocumentInput.addField iterated unmodifiableMap(IdentityHashMap).entrySet() per field per document, allocating an entry-set iterator and walking IdentityHashMap's sparse table on every call (IdentityHashMapIterator.hasNext and UnmodifiableEntrySet frames in bulk-indexing CPU profiles). The map is immutable after construction, so snapshot it once into flat DataFormat[]/DocumentInput[] arrays and loop those in addField/setRowId. JMH (CompositeDocumentInputBenchmark, 3 forks, 5+5): array broadcast is ~10x the map iteration across 5-50 fields and 1-3 secondaries with no-op delegates (isolating broadcast overhead). Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
VSRManager.addDocument resolved each field's vector by name twice per field per document: once for a null-check, then again inside every ParquetField.addToGroup implementation (ManagedVSR.getVector -> HashMap.get was a visible frame in bulk-indexing CPU profiles). Change the ParquetField SPI to pass the resolved FieldVector and row index into addToGroup; VSRManager resolves the vector once, null-checks it, and hands it through. A name-resolving createField overload is kept for tests/callers that don't pre-resolve. The row index is captured once per document (row count only advances after all fields are written). JMH (VSRVectorLookupBenchmark, 3 forks, 5+5, 50 fields): single lookup ~2x the double lookup (98.8 vs 186.8 ns); an ordinal-array layout would be ~10x further and is left as follow-up if Linux ingest re-profiling still shows this frame. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
An ingest allocation profile (60s -e alloc) attributed ~9.7% of total allocation to the per-document dedup set and field list growing incrementally: HashMap$Node per field plus Node[] table resize copies (16 -> 256 for a ~100-field document), with ArrayList growth on top. ParquetIndexingEngine.newDocumentInput now sizes both collections from the Arrow schema's field count, cached against the mapping version so the schema walk only reruns when the mapping changes. Sized so the expected inserts never trigger a resize. JMH -prof gc (200 fields): 18649 -> 14201 B/op (-24%) and 2689 -> 1908 ns/op vs the default-sized path; the remaining bytes are the per-field HashMap$Node and FieldValuePair entries themselves. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
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.
Description
Removes per-field hashing, iteration, redundant lookup, and resize-allocation overhead from the composite/Parquet indexing hot path, motivated by an async-profiler CPU capture of bulk ingest where
JVM_IHashCodewas the single largest self-cost (~10.8%), and a follow-up allocation profile (-e alloc) where the per-document dedup set'sHashMap$Node/Node[]churn was ~9.7% of total allocation.1.
ParquetDocumentInput: dedup fields by name instead of object identityaddFielddetected duplicate single-valued fields with aCollections.newSetFromMap(new IdentityHashMap<>()), so every field of every document paidSystem.identityHashCode(MappedFieldTypeinherits identityhashCode()). That call is theJVM_IHashCodeframe in the profile.The mapping layer forbids two mappers sharing a name (
MappingLookup), and everyaddFieldcaller passesfieldType(), so within one document same name ⇒ sameMappedFieldTypeinstance. Deduping in aHashSet<String>onfieldType.name()is therefore behaviourally equivalent, usesString's cached hash (no identity-hash intrinsic), and stays O(n) — no quadratic scan on wide documents.2.
CompositeDocumentInput: snapshot secondary inputs to arraysaddFielditeratedunmodifiableMap(IdentityHashMap).entrySet()per field per document, allocating an entry-set iterator and walkingIdentityHashMap's sparse table on every call (IdentityHashMapIterator.hasNext/UnmodifiableEntrySetframes). The map is immutable after construction, so the constructor now snapshots it once into flatDataFormat[]/DocumentInput[]arrays andaddField/setRowIdloop those.3.
ParquetFieldSPI: resolve the Arrow vector once per fieldVSRManager.addDocumentresolved each field's vector by name twice per field — once for a null-check, then again inside everyParquetField.addToGroupimplementation (ManagedVSR.getVector→HashMap.get). The SPI now passes the resolvedFieldVectorand row index intoaddToGroup;VSRManagerresolves once, null-checks, and hands it through. A name-resolvingcreateFieldoverload is kept for callers that don't pre-resolve.4. Pre-size the per-document collections from the mapped-field count
The allocation profile showed the dedup set and field list growing incrementally per document (
HashMap$Node[]table copies 16 → 256 for a ~100-field document, plusArrayListgrowth).ParquetIndexingEngine.newDocumentInputnow sizes both from the Arrow schema's field count, cached against the mapping version so the schema walk only reruns on mapping changes.Benchmarks
New
jmhsource sets in both plugins (same wiring as:libs:opensearch-concurrent-queue); all drive the real classes over genuineNumberFieldType/KeywordFieldTypeinstances.IdentityHashMap.hashframe is present in the old strategy and absent with the name-keyed set.-prof gcat 200 fields: 18 649 → 14 201 B/op (−24%) and 2 689 → 1 908 ns/op vs default-sized collections. Remaining bytes are the per-fieldHashMap$Node/FieldValuePairentries themselves; eliminating those needs input pooling or a fused structure — measured trade-off, deferred.Run:
./gradlew -Dsandbox.enabled=true :sandbox:plugins:parquet-data-format:jmh/:sandbox:plugins:composite-engine:jmhwith-Pjmh.includes=<name>.Honesty / scope notes for reviewers
HashSetallocates more per document than the oldIdentityHashMapat small field counts (-prof gc@20 fields: 1 480 vs 592 B/op) becauseHashMapallocates aNodeper entry whileIdentityHashMapis a flat table. The old path's cost was CPU (identity-hash intrinsic + VM transition frames), the new path's cost is short-lived young-gen garbage. The production GC data so far (0 old-gen collections, 12.1 s young-gen over a 51-min ingest) says this is not pathological, but the Linux before/after should watch alloc-rate, not just CPU.KeywordFieldMapper.normalizeValue → AttributeSource.addAttributeJVM_IHashCodesource and the JacksonDupDetectorallocation hotspot (~8.4% of alloc, duplicate-JSON-key detection in parse) are not touched here.Check List
:sandbox:plugins:parquet-data-format:test)