GTFS-RT: constrained repro + Fix 1 (memoize BlockStopTimeList.get)#473
Open
aaronbrethorst wants to merge 5 commits into
Open
GTFS-RT: constrained repro + Fix 1 (memoize BlockStopTimeList.get)#473aaronbrethorst wants to merge 5 commits into
aaronbrethorst wants to merge 5 commits into
Conversation
Follow-up to the PR #472 perf investigation. The baseline there used a single-agency KCM bundle on an unconstrained desktop and could not reproduce the production 85% CPU peg. This rebuilds the reproduction to mirror the actual production container (agency-dockerfiles/test-organization-1): - multi-agency rule-set-19 build-56 bundle (5 agencies) via BuildKcmBundle over an <agencyId>/<feed>.zip input tree - feed pinned to agencyIds=["1"] (PerfBundleHarness now calls setAgencyId; GtfsRealtimeSource.start() otherwise defaults to all agencies) - live KCM feed vs stale build-56 -> a real per-cycle ERROR flood (~52 lines/refresh) - the production [%F:%L] log config, plus cheap/quiet variants for an A/B/C - docker --cpus=1 --memory=2g, TZ=America/Los_Angeles (required; UTC NPEs in BlockFinder service-date math) Results (docs/perf addendum): - The "expensive [%F:%L] logging x flood" hypothesis is REFUTED: config-to-config deltas (~10-25ms) are smaller than run-to-run GC-timing spread (75-155ms). - Even fully faithful and constrained, one 30s poll is ~195-210ms median (~0.7% of a core at 30s cadence) and does not peg. The per-refresh allocation waste is reconfirmed; the 85% peg points to a continuous driver (read traffic / cadence), not the single poll. Adds: PerfBundleHarness agency pinning, perf/logconfigs/log4j2-{prod,cheap,quiet}.xml, perf/run-constrained.sh, perf/README.md, and the findings addendum.
BlockConfigurationEntryImpl$BlockStopTimeList.get(index) rebuilt a new BlockStopTimeEntryImpl on every call — the single largest allocation source on the GTFS-RT poll and arrivals read paths (docs/perf findings, Fix 1: ~59-69% of all allocation). The wrapped inputs are fixed for the bundle's lifetime, so the wrappers are effectively immutable. Populate a lazy, transient BlockStopTimeEntry[] cache instead (only touched indices retained; transient so bundle serialization is unchanged). Thread-safe without locking: BlockStopTimeEntryImpl has only final fields, so a racing reader sees either null (rebuild an equivalent wrapper) or a safely-published instance. Measured before/after on the constrained multi-agency harness (quiet config, --cpus=1 --memory=2g, warmup 8/measure 40, 3 reps each): - allocation/refresh: 542 MB -> 159 MB (-71%) - p50 refresh: 188 ms -> 143 ms (-24%) - mean/p95: GC-pause-timing noise on desktop hardware (both before and after hit occasional ~1.5s pauses); the 71% alloc cut should convert to lower GC CPU on a more memory-pressured/slower box. Tests: adds BlockConfigurationEntryImplTest (values correct + elements memoized; fails pre-fix on identity, passes post-fix). ScheduledBlockLocationServiceImplTest (14) and the golden GtfsRealtimeTripLibraryCharacterizationTest pass unchanged. Adds a ThreadMXBean allocation counter to GtfsRtRefreshBenchmark and an EXTRA_CP hook to run-constrained.sh for measuring a rebuilt module without mvn install.
Per /simplify altitude review: PerfBundleHarness.open() read
System.getProperty("perf.agencyIds") directly, unlike every other harness
input (bundle root, feed URLs) which GtfsRtRefreshBenchmark.main() parses and
passes via constructor. Move the parse to main(), pass a List<String> through a
new 5-arg constructor (4-arg overload delegates with no pinning, keeping
StopCountHistogram untouched), and use the bulk setAgencyIds(List) setter
instead of a per-token setAgencyId loop. Behavior unchanged: pinned agencyIds=[1],
matched=373/398.
…tations From /pr-review-toolkit:review-pr: - pr-test-analyzer (Important): the cache field's 'transient' is load-bearing because BlockStopTimeEntryImpl is not Serializable, and configs are serialized after their caches populate during block-index computation. Add populatedCacheSerializesCleanly() -- verified it fails with NotSerializableException when 'transient' is removed, passes with it. Refactor the fixture into a shared helper. - comment-analyzer: replace brittle cross-repo 'Dockerfile line 4/8' citations (external agency-dockerfiles repo; line numbers rot silently) with the setting name in log4j2-prod.xml, run-constrained.sh, and the findings addendum. code-reviewer and silent-failure-hunter: clean, no findings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds constrained GTFS-RT benchmark tooling with configurable logging, agency filtering, and allocation reporting; memoizes block stop-time entries; adds regression tests; and documents production-faithful reproduction results and validation metrics. ChangesGTFS-RT performance investigation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@onebusaway-gtfsrt-perf/src/main/java/org/onebusaway/perf/gtfsrt/GtfsRtRefreshBenchmark.java`:
- Around line 90-102: Update threadAllocatedBytes() to enable thread
allocated-memory monitoring before calling getThreadAllocatedBytes(...): when
the bean is a com.sun.management.ThreadMXBean and
isThreadAllocatedMemorySupported() is true, check
isThreadAllocatedMemoryEnabled() and call setThreadAllocatedMemoryEnabled(true)
if needed, then perform the measurement while preserving the existing fallback
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d0e819fe-3fd7-44a6-9f38-9215e8eb533d
📒 Files selected for processing (10)
docs/perf/2026-07-10-gtfsrt-matching-findings.mdonebusaway-gtfsrt-perf/perf/README.mdonebusaway-gtfsrt-perf/perf/logconfigs/log4j2-cheap.xmlonebusaway-gtfsrt-perf/perf/logconfigs/log4j2-prod.xmlonebusaway-gtfsrt-perf/perf/logconfigs/log4j2-quiet.xmlonebusaway-gtfsrt-perf/perf/run-constrained.shonebusaway-gtfsrt-perf/src/main/java/org/onebusaway/perf/gtfsrt/GtfsRtRefreshBenchmark.javaonebusaway-gtfsrt-perf/src/main/java/org/onebusaway/perf/gtfsrt/PerfBundleHarness.javaonebusaway-transit-data-federation/src/main/java/org/onebusaway/transit_data_federation/impl/transit_graph/BlockConfigurationEntryImpl.javaonebusaway-transit-data-federation/src/test/java/org/onebusaway/transit_data_federation/impl/transit_graph/BlockConfigurationEntryImplTest.java
isThreadAllocatedMemorySupported() only checks JVM support; the measurement is disabled by default per the JDK spec, so getThreadAllocatedBytes() can return -1 on a capable HotSpot JVM that has it off (it happened to be on for the runs in this PR). Enable it (idempotently) before reading so the ALLOC report is robust across JVMs. Verified ALLOC still reports 165.6MB/refresh.
Member
Author
|
Addressed the CodeRabbit review:
|
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
1 participant
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.
Follow-up to #472. Two things: a production-faithful reproduction of the GTFS-RT poll path, and the implementation of that report's top-ranked fix.
Summary
rule-set-19build-56 bundle — 5 agencies; live KCM feed producing a real per-cycle ERROR flood; the production[%F:%L]log config;docker --cpus=1 --memory=2g;TZ=America/Los_Angeles). Result: the tempting "expensive[%F:%L]location logging × flood" hypothesis is refuted (A/B/C over prod/cheap/quiet log configs — deltas are smaller than run-to-run GC noise), and even fully faithful + constrained, one 30 s poll is ~0.7 % of a core — i.e. not the 85 % peg. Evidence points to a continuous driver (API read traffic / cadence), not the single poll.BlockConfigurationEntryImpl$BlockStopTimeList.get(). It rebuilt aBlockStopTimeEntryImplon every call — the largest allocation source on the poll and arrivals-read paths. Now a lazytransientcache (thread-safe without locking: the entry has only final fields). Measured on the constrained harness (3 reps): allocation/refresh 542 MB → 159 MB (−71 %), p50 refresh 188 ms → 143 ms (−24 %); mean/p95 are GC-timing noise on desktop HW. Also lightens the arrivals read path (sharedgetBlockLocation→.get()).onebusaway-gtfsrt-perf: benchmark with a per-thread allocation counter, agency-pinned harness,run-constrained.sh, log4j2 A/B/C configs) and the findings write-up (docs/perf/2026-07-10-gtfsrt-matching-findings.md, Addendum §A–E).Test plan
BlockConfigurationEntryImplTest— values correct, elements memoized (assertSame), andpopulatedCacheSerializesCleanly(guards the load-bearingtransient; verified it fails withNotSerializableExceptionwhentransientis removed, passes with it)ScheduledBlockLocationServiceImplTest(14) and goldenGtfsRealtimeTripLibraryCharacterizationTestpass unchangedimpl.transit_graph+impl.blocks+impl.realtimepackages greenGtfsRealtimeSource.refresh(): matched 373/398, alloc 159 MB/refresh post-fixlicense:checkpasses on both modulesReview notes (non-blocking)
http_request_countand deployedrefreshInterval, then a concurrent read-load reproduction againstgetBlockLocation(now cheaper post-Fix 1) — not done here.parseCsv.Summary by CodeRabbit