joewitt commented on PR #11164: URL: https://github.com/apache/nifi/pull/11164#issuecomment-5648132513
# Follow-on notes from validating NIFI-15862 (virtual-thread scheduling) These notes are **not** a review of the scheduling-agent change itself. I have been running Mark’s `nifi-15862-virtual-threads` work (`NIFI-15862` / https://github.com/apache/nifi/pull/11164) as the baseline, with the goal of seeing whether virtual threads actually raise FlowFile/sec on a CPU-bound loop — and then whether leftover engine hotspots, once VT is in play, are worth a **later, separate PR**. Nothing below is meant to block or expand this PR. The lock / map / comparator patches are local follow-on experiments. ## What we were trying to learn 1. Does VT scheduling keep the engine busy on a flow that is **not** disk-bound (0-byte FlowFiles, volatile repos)? 2. When we stack independent Generate → UpdateAttribute loops, does throughput **add**, or do the pairs steal from each other? 3. If they steal, is that the scheduler, virtual-thread pinning, or an older engine lock/allocation path that VTs just make more obvious? ## Test setup - **Build:** NiFi `2.12.0-SNAPSHOT` on branch `nifi-15862-virtual-threads` - **JDK:** Azul Zulu 25, macOS, 16 cores - **Heap:** 16g (`-Xms16g -Xmx16g`) - **Repos (deliberate, so we are not measuring WAL/content disk):** - `nifi.flowfile.repository.implementation=org.apache.nifi.controller.repository.VolatileFlowFileRepository` - `nifi.provenance.repository.implementation=org.apache.nifi.provenance.VolatileProvenanceRepository` - **Scheduling:** `nifi.scheduling.strategy=AUTO` (virtual threads on this JDK), controller `maxTimerDriven=64` - **Flow:** one process group with independent pairs of GenerateFlowFile → UpdateAttribute - 0-byte FlowFiles - Generate batch size 1000 - no user prioritizers on the connections - pair 4 left stopped; three pairs running for the stacked tests - concurrent tasks were not uniform (one UpdateAttribute was at 20, others at 5; Generates at 4–5). Extra concurrent tasks on a single pair did **not** move the needle much once the pair was already saturated. - **What we refused to do for the benchmark:** skip provenance event construction, NoOp provenance, or otherwise disable work the engine would do in a real flow. Volatile repos only avoid *persistence*, not in-memory event/attribute work. ### How we measured NiFi’s processor status `flowFilesOut` is a **5-minute rolling window**. Dividing that by 300 is a decent steady-state rate after the window is full; a 30-second delta of those counters is **wrong** once the window is sliding (old seconds drop off → negative “rates”). After a restart, 30-second deltas of the filling 5-minute counters *are* usable. We therefore treated: - 5-minute Generate out / 300 as the long-run rate - 30-second deltas only when the window was filling from a restart, or when we knew the counters were monotonic - `top` / load average / heap from system-diagnostics as the machine picture - JFR (`profile` settings) plus park-stack aggregation to see *why* CPU stopped scaling ## Results on Mark’s code (before any follow-on patches) | Configuration | Approx. Generate throughput | Notes | |---|---|---| | 1 pair | ~163k FF/s, later ~252k FF/s | same loop; later number after the JVM/flow had warmed | | 2 pairs | ~526k FF/s (about 252k + 274k) | **adds** — this is the encouraging VT result | | 3 pairs, extra threads | **did not add**; pairs stole from each other | CPU ~660%, disk idle, ~30% sys time | So: virtual threads **do** let two independent CPU-bound loops run together. A third pair did not buy a third of the machine; the ceiling was not disk and not “we ran out of platform threads.” ### JFR (3-pair steal, still fair queue locks) Ruled **out**: - content/repo disk (idle) - virtual-thread **pinning** (no pinned-thread events in the recording) Ruled **in** (hot / park stacks): - `ReentrantReadWriteLock` **fair** path (`hasQueuedPredecessors`) on `SwappablePriorityQueue` put/poll — VT park convoy on every FlowFile - provenance attribute handling (`unmodifiableMap` / enrich) - `PriorityQueue.siftDown` / `QueuePrioritizer.compare` - ConcurrentHashMap / UpdateAttribute attribute-map churn The scheduler itself was not the thing we were parked on. ## Follow-on changes (local, for a later PR) All of these sit on top of NIFI-15862. They are engine-path tweaks that VTs made expensive because many more tasks actually run the put/poll path at once. ### 1. Non-fair lock on `SwappablePriorityQueue` — **this is the one that mattered** **Change:** `new ReentrantReadWriteLock(true)` → `new ReentrantReadWriteLock()`. **Why:** FlowFile **order** is the heap comparator (penalty → user prioritizers → content claim → id), not lock-acquisition FIFO. Fairness was only ordering *waiters*. With virtual threads, every put/poll that lost the fair lock paid a park/unpark handoff. Three pairs convoyed on that. **Risk we accepted:** barge-in / theoretical waiter starvation on a tiny critical section (heap op + size counters). UI/status `readLock` snapshots can wait a bit longer under a write storm; same class of issue as any non-fair RW lock. We do **not** lose queue ordering semantics. **Result:** three pairs went to **~607k FF/s** combined (was fighting itself below the two-pair **~526k**). JVM CPU dropped some (~645% → ~563% in one sample; later samples still ~650% depending on heap/GC). Fair-lock scheduling disappeared from the post-fix profile. This is the change I would most like considered for a follow-on PR. It is small, and it is specifically more important once VT increases concurrent put/poll. ### 2. Non-fair lock on `StandardFlowFileQueue` — consistency only **Change:** same `true` → default (non-fair). **Why:** leftover twin of (1). **Put/poll never take this lock**; they go to `SwappablePriorityQueue`. The outer lock is only `session.get(FlowFileFilter)` across **multiple incoming connections** (lock-order deadlock avoidance) and selective drops. **Result:** not re-benchmarked on purpose. This Generate → UpdateAttribute loop never acquires it (`get()` without a filter does not lock-all-queues). No expected throughput change. Fine as the same cleanup if we touch (1). Note for later, not this experiment: `lock()` does not wrap the inner queue, so “lock all queues” does not freeze puts/polls on the heap. Separate design leftover. ### 3. Freeze FlowFile / provenance attribute maps once (`Map.copyOf`) **Change:** - `StandardFlowFileRecord` constructs a frozen map once; `getAttributes()` returns that instance instead of wrapping with `Collections.unmodifiableMap` on every call. - Builder `fromFlowFile` still aliases the frozen map and copy-on-writes on mutate (same StackOverflow-avoidance story as today, without wrapping UnmodifiableMap in UnmodifiableMap). - `StandardProvenanceEventRecord` previous/updated attribute maps likewise `Map.copyOf` at construct. **Why:** JFR showed map wrap/copy on the provenance/session path. Intentionally **not** “skip building the event.” **Result:** after this plus (4), 30s Generate rates were **~293k + 158k + 162k ≈ 613k FF/s** vs a 5-minute baseline of **~281k + 157k + 157k ≈ 595k** immediately before the deploy. That is **in the noise** of the ~607k we already had from (1). Pair 1 stayed much hotter than pairs 2 and 3 (concurrent-task mismatch, not a comparator bug). Worth keeping as a real cleanup (and it matches the existing “don’t wrap the map N times” comment), but it is **not** the VT scaling fix. ### 4. `QueuePrioritizer` fast path for the default order **Change:** if there are no user prioritizers, neither FlowFile is penalized, and both content claims are null, compare `contentClaimOffset` then `id` with `Long.compare`. Otherwise the existing chain (penalty → user prioritizers → claim → id). **Why:** `PriorityQueue.siftDown` was hot; default order on this flow is claim-null + unpenalized, so id (after offset) decides. We did **not** replace `PriorityQueue` with `ArrayDeque`: default order is **not** arrival order (penalty → claim/offset → **id**). **Result:** bundled with (3); no clear extra win on this flow. ## Tests run on the follow-on patches - `TestSwappablePriorityQueue` (29) - `TestStandardFlowFileQueue` (20) - `QueuePrioritizerTest` (7, including default-order / penalty / claim still honored) - `TestStandardFlowFileRecord` (frozen map + UOE on mutate) - `StandardProvenanceEventRecordTest` (2) All passed. Not a substitute for a clustered load-balance run or a flow that uses user prioritizers / penalties heavily. ## What I would take from this for NIFI-15862 vs a later PR - **This PR’s VT scheduler looks like it is doing its job** on a CPU-bound, 0-byte loop: two pairs **add**. We did not see pinning. We did not see the scheduler as the 3-pair ceiling. - The 3-pair ceiling was **fair `SwappablePriorityQueue` locking** interacting badly with many virtual threads pounding put/poll. That lock predates VT; VT just lights it up. - I would **not** fold the follow-on patches into NIFI-15862. They are easier to reason about (and revert) on their own, and only (1) moved throughput. - If we do a follow-on, start with (1), optionally (2) as the same one-liner family. (3) and (4) are optional / lower value on this particular flow. - Remaining engine cost after (1), still visible in JFR and still **not** disk: provenance event construction/enrichment, attribute maps, `PriorityQueue` compares. We left those honest on purpose. ## Open questions / caveats - Numbers are one Mac, 16 cores, volatile repos, 0-byte files. A WAL + persistent provenance + real content path will look different (and should). - Pair imbalance (one loop ~2× another) is at least partly concurrent-task settings, not fully investigated. - `StandardFlowFileQueue`’s outer lock vs inner lock split is a correctness/clarity issue for `get(FlowFileFilter)`, independent of fairness. - We should still compare **TIMER_DRIVEN (platform threads)** vs **AUTO/VT** on the *same* flow and same machine if we want a clean “VT vs old scheduler” number. The work above was “VT on, then remove engine bottlenecks VT exposed,” not a head-to-head A/B of the two scheduling agents. -- 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]
