Repository navigation
Build block indexes from each trip's ends - #1673
Closed
aaronbrethorst wants to merge 13 commits into
Closed
aaronbrethorst wants to merge 13 commits into
aaronbrethorst wants to merge 13 commits into
Conversation
Upcoming work on #1657 rewrites the static GTFS import. Each change has to show that the database it builds is the same as before, or differs only as intended. TestParityTables imports RABA and compares one digest per table with a committed golden file. Autoincrement keys and import timestamps are left out, so two imports of the same feed give the same digests. Virtual tables and their shadow tables are left out too; they are derived from the tables that are compared. block_trip_entry, the one table with a foreign key to an autoincrement ID, is compared through its index's natural key. Any new table like it fails the test until it gets the same treatment. When the digests differ, the test writes each table's normalized rows to a temporary directory, so the rows can be diffed against a run of main. Regenerate the golden file with -update-parity.
Upcoming work on #1657 and #1660 changes the import and the order of block trips. Table digests show whether the database changed; this shows whether what clients receive changed. TestParityAPI sends a fixed set of requests, one or more per endpoint family, at a pinned time, and compares each response with a committed golden file. Numbers are kept as written, and reference lists are sorted, because they are built from maps. The requests then run again with PRAGMA reverse_unordered_selects, which makes SQLite return the rows of a query without ORDER BY in reverse. A response that changes under it relies on row order that nothing guarantees. That pass only compares and never writes the golden files. The three agency list endpoints do so today (#1661) and are skipped in that pass until it is fixed. createTestApiWithFeedAndClock is createTestApiWithFeed with a clock, so the test can pin the time.
The block golden file failed on CI (amd64) but passed on arm64. Go fuses multiply-adds on arm64, so computed values such as distanceAlongBlock differ in the last bits between the two. Non-integer numbers are now compared at 12 significant digits, which is well below a millimetre for distances. Integers, including timestamps, are still compared exactly.
#1657 needs a number to judge each memory change against. The existing reload perftest reports one process-wide peak and uses a TriMet feed only. TestImportMemory imports any local feed (MAGLEV_PERF_FEED) into a new database. A slog handler treats each operation the importer logs as the start of a stage, and a sampler reads the heap through runtime/metrics, which doesn't stop the world, and records the peak per stage. Warnings and errors still reach the test output. The test also reports the process's peak RSS, which includes SQLite's own memory, and cgroup memory.peak when it runs in a container. Reading the feed and parsing it logged nothing, so their memory was charged to whatever came before. They now log reading_gtfs_data and parsing_gtfs_data at debug level, like the import's other stages. For NJ Transit bus on main, the parse peaks at 578 MiB of heap, the stop_times insert at 1.97 GiB, and the process at 2.34 GiB RSS. It runs only under the perftest build tag, like the other perftests.
buildBlockLayoverIndex sorts each block's trips by the departure time of their first stop. When two trips leave at the same time, their order came from the row order of trips.txt and an unstable sort, and that order decides which trip a layover leads into. The streaming import in #1657 changes the row order, so the layovers could change with it. Ties are now broken by trip ID. The RABA parity golden files are unchanged: RABA has no such ties. Closes #1658
v1.3.0 finds a CSV column whose header name has spaces around it, so a feed that writes "stop_id, stop_name" in its header rows imports its stops and routes instead of failing validation. It doesn't trim spaces in data cells; a feed that pads its values too still loses rows that refer to each other by ID. v1.3.0 also adds realtime fields (Alert.IsDeleted, Trip.Timestamp, Trip.EntityIndex). Maglev doesn't read them yet, so they change nothing here; #1664 tracks using IsDeleted. The streaming import in #1657 will need a later go-gtfs release. This bump lands on its own first, so the header change isn't mixed into a refactor that must not change behavior. The RABA parity golden files are unchanged.
After a changed import, the direction precomputer read every row of the shapes table into one Go map before computing stop directions. For NJ Transit bus that is 3.3 million points, and the map alone held hundreds of MB. Once the import itself is fixed (#1657), this would set the peak. The precomputer now keeps the most recently used shapes up to 500,000 points (about 20 MB of rows), and reads others with the per-shape query the calculator already used when no cache was set. The budget counts points, not shapes, because shape sizes vary widely between feeds. Lookups of one shape share a single read, and the completion log reports cache hits and reads. The query returns the same points in the same order (ordered by shape_pt_sequence), so stop directions are unchanged: the RABA parity digests, which include stops.direction, still match. On NJ Transit, the precompute holds about 580 MiB of heap while it runs instead of about 1.5 GiB, and the import takes as long as before (about 21 s). Without any cache it took 31 s, because each shape was read again for every stop on it. A feed whose stops visit shapes in a scattered order gets fewer hits, but never more memory than the budget. The full shape map was set only by the precomputer, so SetShapeCache and its lock are removed; tests now seed the bounded cache instead. GetAllShapes had no other caller and is removed.
StoreGtfsData built one slice of every stop time in the feed, and bulkInsertStopTimes then built every multi-row INSERT, with each value boxed in an []any, before it ran the first one. For NJ Transit bus (1.8 million stop times) that is two more full copies of the stop_times on top of the parsed feed, and the import's peak. stopTimeRows now yields each row from the parsed trips only when it is needed, and insertInBatches inserts the rows as they arrive, holding one batch at a time. Full batches reuse one prepared statement. Rows go in in the same order as before, so the RABA parity digests are unchanged. On NJ Transit, the stop_times stage's peak heap falls from 1.97 GiB to about 810 MiB, the process's peak RSS from 2.34 GiB to 2.05 GiB, and the import gets slightly faster (about 19 s). The shapes insert is next; it can reuse insertInBatches.
SonarCloud's dynamic-SQL rule flags the two calls that run the batch INSERT, because the statement is built at run time. It is built only from the fixed table and column names and "?" placeholders, and every value is bound, so there is nothing to inject. The calls now carry NOSONAR markers with a comment saying why.
Shapes had the same pattern stop_times had: StoreGtfsData built one slice of every shape point, and bulkInsertShapes built every multi-row INSERT before it ran the first. NJ Transit bus has 3.3 million shape points, so the shapes stage peaked at 1.35 GiB of heap. Shapes now go through the same insertInBatches helper as stop_times, fed by shapeRows, and bulkInsertRows holds the logging and transaction handling both tables share. Rows go in in the same order and with the same shape_pt_sequence numbering, so the RABA parity digests are unchanged. On NJ Transit, the shapes stage peaks at 639 MiB, and the process's peak RSS falls from 2.05 GiB to 1.0 GiB (2.34 GiB on main).
ReloadStatic held the parsed feed in a local variable that it read again after the import, so the whole feed stayed reachable while stop directions were precomputed. For NJ Transit bus that kept several hundred MB alive through a stage that needs about a hundred. storeConfiguredFeed now loads, parses and stores the feed, and the parsed copy is reachable only from there. ReloadStatic precomputes directions after it returns, first calling debug.FreeOSMemory so the feed's memory is collected and returned to the OS instead of waiting for the GC to get around to it. importStaticIntoDB, which did the store and the precompute together, is replaced by precomputeStopDirections. On NJ Transit, the precompute starts at 113 MiB of heap instead of about 630 MiB, and the process's peak RSS falls from 1.0 GiB to about 950 MiB. The peak is now the parse itself (about 555 MiB of heap), which only a streaming parser (#1657, step 3) can lower.
Every startup and every daily reload read the whole static feed into memory and parsed it, and only then did StoreGtfsData compare its hash with import_metadata. So a restart with an unchanged feed and an existing database reached nearly the same memory peak as a first import. fetchFeedFile now puts the feed in a local file while hashing it: a local feed is read where it is, and a remote one is downloaded next to the database rather than to a temporary directory that may be held in memory. Downloads left behind by a crash are removed first. storeConfiguredFeed asks the new Client.HasImported whether the database already holds that hash from that source, and parses the feed only when it doesn't. HasImported and StoreGtfsData share one isSameFeed rule. The hash is still the SHA-256 of the feed bytes, so existing databases match and don't re-import. parse hashes the bytes it actually reads, so the stored hash always describes the stored data. With gtfstidy on, the hash is marked "+gtfstidy" only when tidying succeeded: turning tidy on or off causes a re-import, and a failed tidy is retried on the next reload instead of being recorded as done. A database imported with tidy on re-imports once. The static download now clones http.DefaultTransport, like the realtime client, so it honors proxy settings from the environment. For NJ Transit bus with an existing database, a restart is healthy in 1 s with a peak RSS of 194 MB. On main it takes 4 s and 825 MB.
buildBlockTripIndex and buildBlockLayoverIndex read every trip's full stop times, but use only the first and last of them. A streaming import (#1657) never holds all stop times at once, so it can't hand these builders a fully parsed feed. Both builders now take tripEnds: a trip's route, service and block, and its first and last stop and times. tripEndsOf builds them from a parsed feed, in trip order, for the import as it is today; a streaming import can collect them as each trip's stop times go by. The grouping, sorting and layover rules don't change, and the RABA parity digests for block_trip_index, block_trip_entry and block_layover are unchanged.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
This was referenced Oct 20, 2026
Closed
aaronbrethorst
force-pushed
the
ab/1657-hash-first
branch
from
October 20, 2026 07:39
c595935 to
749706f
Compare
Member
Author
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.
Part of #1657 (groundwork for step 13, the streaming import). Stacked on #1672. No behavior change.
Why
buildBlockTripIndexandbuildBlockLayoverIndextook the whole parsed feed and read every trip's full stop times, but they use only the first and last of them. The streaming import never holds all stop times at once, so it can't hand these builders a fully parsed feed.What
[]tripEnds: each trip's route, service and block, plus its first and last stop and times.tripEndsOfbuilds them from a parsed feed, in trip order, for today's import. The streaming import will collect them as each trip's stop times go by.buildBlockTripIndex, the oldtripInfostruct, which copied a subset of these fields, is gone.The grouping, sorting and layover rules don't change. The RABA parity digests for
block_trip_index,block_trip_entryandblock_layoverare unchanged.TestTripEndsOfcovers three cases: