deepthi912 commented on code in PR #19596:
URL: https://github.com/apache/pinot/pull/19596#discussion_r4127328488
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -426,15 +434,15 @@ protected GenericRow doUpdateRecord(GenericRow record,
RecordInfo recordInfo) {
if (!recordInfo.isDeleteRecord()
&&
recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue())
>= 0) {
IndexSegment currentSegment = recordLocation.getSegment();
- ThreadSafeMutableRoaringBitmap currentQueryableDocIds =
currentSegment.getQueryableDocIds();
int currentDocId = recordLocation.getDocId();
- if (currentQueryableDocIds == null ||
currentQueryableDocIds.contains(currentDocId)) {
+ // Read lock: currentSegment cannot be destroyed while LazyRow
reads its columns. A consuming segment needs
+ // no lock: it is destroyed only after
replaceSegment()/removeSegment() has moved or dropped every location
+ // pointing at it, and those run under the same per-key compute as
this read.
+ if (tryAcquireSegmentReadLock(currentSegment)) {
Review Comment:
Good call-out — this is the right thing to validate. Status, including what
I don't yet have.
**Structural point.** Each partition has one consumer thread and
`_destroyLock` is per segment, so on the steady-state ingestion path the read
lock is uncontended. The contention that does exist is with `destroy()`: it's a
*non-fair* `ReentrantReadWriteLock`, so once a `destroy()` writer is queued,
`readerShouldBlock()` parks new readers — and the consumer is inside a
`computeIfPresent` bin lock at that moment. That's the case worth measuring,
and my current harness does not produce it.
**CPU — measured, but not yet conclusive.** JMH over a real 200K-row
segment, comparing the previous-row read with and without the guard, varying
columns carried forward (3 forks, 30 measurement iterations):
| columns | unguarded | guarded | delta |
|---|---|---|---|
| 1 | 138.211 ± 1.209 | 140.013 ± 0.947 | +1.8 ns (+1.3%) |
| 4 | 497.167 ± 19.133 | 465.753 ± 3.257 | −31.4 ns |
| 8 | 893.593 ± 6.880 | 925.437 ± 18.095 | +31.8 ns (+3.6%) |
The guard in isolation measures ~16.2 ns.
I'm not drawing a conclusion from this. The 4-column row shows the guarded
path as *faster*, which is impossible — the delta (~2–3% of the operation) is
below the harness's resolution, and more forks only tightened the bars around
noise. The harness also has fidelity gaps pulling both ways: it calls
`ImmutableSegmentImpl#tryAcquireReadLock()` directly rather than through
`tryAcquireSegmentReadLock(IndexSegment)`, missing the bimorphic `instanceof`
dispatch (under-reports); and its denominator omits
`PartialUpsertHandler.merge(...)`, the `queryableDocIds` check, the enclosing
CHM bin lock, and null handling (over-reports). No partition manager is in the
loop at all.
The one point that reproduced across runs is 1 column: **+1.8 to +2.3 ns**,
consistently positive and outside the error — about 8× below the guard's
standalone cost, which would suggest the CAS largely overlaps with the
column-read work. I want a correct harness before claiming that.
**Memory.** One `ReentrantReadWriteLock` per `ImmutableSegmentImpl`,
measured at **121 bytes/instance** (2M allocations, compressed oops, G1). At
10K segments that's **~1.2 MB**, against segments that are megabytes each —
under 0.01% of segment footprint. The `volatile boolean _destroyed` fits
existing padding. The only part that grows is RRWL's `ThreadLocalHoldCounter`:
one `ThreadLocalMap` entry per acquiring thread per lock. Acquirers are the
partition's consumer thread and the segment-replace thread — query threads
never take it — so it's bounded by segments-per-partition, not thread count.
If footprint matters more than I think, the lighter shape is an
`AtomicInteger` reader count with a destroyed sentinel: one int field, no lock
object graph, no ThreadLocal. I don't think 1.2 MB justifies the extra
complexity.
**Next.** Measure the guard and the merge as separate terms and report the
ratio, rather than subtracting two large noisy numbers; add a proper baseline
arm; route through the production helper; and add a contended arm with a
background `destroy()` writer. I'll post numbers when they're defensible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
