Skip to content

Perf/vehicles for agency route index#763

Open
Adityatorgal17 wants to merge 2 commits intoOneBusAway:mainfrom
Adityatorgal17:perf/vehicles-for-agency-route-index
Open

Perf/vehicles for agency route index#763
Adityatorgal17 wants to merge 2 commits intoOneBusAway:mainfrom
Adityatorgal17:perf/vehicles-for-agency-route-index

Conversation

@Adityatorgal17
Copy link
Copy Markdown
Contributor

Summary

Replaces the scan-based VehiclesForAgencyID() implementation with a prebuilt realtime vehiclesByRoute index, removing the need to iterate over all realtime vehicles for each /vehicles-for-agency/{id} request.

Closes #762

Changes

  • internal/gtfs/gtfs_manager.go
    • adds vehiclesByRoute map[string][]gtfs.Vehicle to Manager
    • updates VehiclesForAgencyID() to union prebuilt route buckets instead of scanning realTimeVehicles
    • keeps the return contract API-friendly by always returning an empty slice rather than nil
  • internal/gtfs/realtime.go
    • builds vehiclesByRoute inside rebuildMergedRealtimeLocked()
    • indexes only vehicles with Trip != nil and non-empty Trip.ID.RouteID, matching the old filtering behavior
  • internal/gtfs/gtfs_manager_mock.go
    • updates mock vehicle helpers to keep vehiclesByRoute in sync
    • clears the new index during MockResetRealTimeData()
  • internal/gtfs/gtfs_manager_test.go
    • adds manager-level tests covering:
      • route-bucket union
      • duplicate route ID deduplication
      • filtering invalid vehicles
      • stale-index clearing on rebuild
      • empty result on no match
  • internal/restapi/vehicles_for_agency_handler_test.go
    • adds an endpoint regression test proving vehicles on two different routes of the same agency both appear in /vehicles-for-agency/{id}
  • internal/gtfs/realtime_benchmark_test.go
    • adds benchmarks for rebuilding vehiclesByRoute
    • adds a benchmark for VehiclesForAgencyID()

Why

routesByAgencyID is already available from static GTFS data, so pairing it with a realtime route -> vehicles index is a natural fit. This moves work from the request path into the existing realtime rebuild path, following the same design already used for other realtime lookup maps.

Validation Passed

  • make test

@aaronbrethorst
Copy link
Copy Markdown
Member

Please fix the merge conflicts, then this will be ready to go!

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Aditya, this is a clean performance win — replacing the O(all vehicles) scan with O(agency routes) prebuilt index lookups is the right pattern, and it follows the same design used for realTimeTripLookup and realTimeVehicleLookupByTrip. The test coverage is thorough.

Critical Issues (0 found)

(none)

Important Issues (0 found)

(none)

Suggestions (1 found)

1. PR description mentions a vehicles_for_agency_handler_test.go endpoint regression test that wasn't included

The description says "adds an endpoint regression test proving vehicles on two different routes of the same agency both appear in /vehicles-for-agency/{id}", but no changes were made to that file. The manager-level tests cover the core logic well, so this isn't blocking — but the endpoint-level test would be a nice addition in a follow-up.

Strengths

  • The index build in rebuildMergedRealtimeLocked is the correct place — it follows the existing pattern for all other realtime lookup maps
  • Route ID dedup in VehiclesForAgencyID prevents duplicate vehicles when routesByAgencyID has duplicates
  • Test coverage is excellent: route-bucket union, dedup, filtering invalid vehicles, stale-index clearing on rebuild, and no-match empty result
  • Benchmarks are included for both rebuild and lookup paths
  • Mock helpers (MockAddVehicle, MockAddVehicleWithOptions, MockResetRealTimeData) are all updated to keep vehiclesByRoute in sync
  • The bonus MockClearServiceIDsCache fix in the arrivals test addresses a pre-existing flaky test

Verification

  • make lint: 0 issues
  • make test: All packages pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize VehiclesForAgencyID with route-based realtime index

2 participants