Reduce number of AgencyId instances

[skjolber]

Less memory usage for huge ShapePoint (2 GB) files.

Add "interning" of AgencyId for entity types which repeat the same id many times.

• Shape
• Trip
• .. more / less?

Works best if String interning is disabled, but even when enabled it seems to save a good chunk of memory. Performance is approximately as before.

Added a few utils classes the help profile memory (can be deleted later).

TODO

• assuming use of non-thread-safe HashMap is acceptable
• assuming test data is representative

[leonardehrenfried]

This is very promising - thanks!

Since the last round of optimizations, I've identified the very inefficient storage of shape points as a major driver of OTP memory consumption and have reworked it in OTP: opentripplanner/OpenTripPlanner#6752

You could check out ShapePointArray so save some memory.

[leonardehrenfried]

BTW, how big are the memory savings in your tests?

[skjolber]

Just printing the memory via those small utils seem to give about the same as profilers. This is just parsing the shape file.

ParseShapePrintMemory:

Total Memory: 11136 MB
Free Memory: 7236 MB
Used Memory: 3899 MB

LegacyParseShapePrintMemory:

Total Memory: 13792 MB
Free Memory: 6595 MB
Used Memory: 7196 MB

ParseShapeStringInterningPrintMemory

Total Memory: 7680 MB
Free Memory: 3275 MB
Used Memory: 4404 MB

LegacyParseShapeStringInterningPrintMemory

Total Memory: 8864 MB
Free Memory: 3689 MB
Used Memory: 5174 MB

Kind of surprising to see that string interning is worse off. Should try this with parsing all files too.

[skjolber]

Maybe you could double the check the numbers here, my tests seems to indicate String interning is no longer necessary.

According to ShapeSingleShotBenchmark, enabling String interning slows down the parsing considerably, so if this holds, this is a double win.

[skjolber]

The above memory usage numbers are straight after parsing, but before freeing up the reader and such.

This table is after all is done, keeping just the GtfsRelationalDaoImpl in the code, and with setPackShapePoints(true)

String intern	Agency intern	Mem total	Mem used
false	true	4752	1116
true	true	4640	1116
false	false	15360	4418
true	false	6944	1893

So still seems like an improvement in memory usage (and skipping interning all together is still an option).

[skjolber]

For the whole feed:

String intern	Agency intern	Trip intern	Mem total	Mem used
false	true	true	7520	2218
true	true	true	7632	2020
false	false	false	18960	5549
true	false	false	9984	2802

I was expecting more memory use in general, but these are the measurements so far.

Maximum memory use must be taken with a grain of salt, not necessarily so important or accurate.

[leonardehrenfried]

Again, this looks like excellent, methodical work.

Unfortunately, I'm at the OTP conference next week so this will have to wait a bit longer.

However, so far I can say that I would like to enable the most aggressive memory optimizations.

Some side notes:

• This library sometimes implements its own version of String.intern(). Presumably there was a problem with it 10+ years ago, but these days are long gone, so I think we can just use the JVM functionality.
• This library also has an absurd number of indirections. Often I don't see the purpose at all so feel free to cut down on it, if you wish.
• There are lots of other questionable design decisions that make no sense to me, like ShapePointProxy rather than an interface with two implementations. Don't feel that these things are set in stone and change them liberally, if you wish.
• Lots of things are configurable, for example the shape point packing, which complicates the code. I'm fine with this library becoming more opinionated.

[leonardehrenfried]

I'm back from the conference and can review again if you want to get this into a reviewable state - perhaps rebase on top of main.

[skjolber]

It seems that by selectively configuring string interning per entity, i.e. disabling it for ShapePoint, a lof of the overhead is removed. If shapeId is interned (as AgencyAndId), there is nothing to be gained from interning the rest of the fields, as they are just to-be-parsed numbers.

[skjolber]

Disabling interning for shapes:

Benchmark                                                   Mode  Cnt   Score   Error  Units
ShapeSingleShotBenchmark.testParse                            ss    2  11,799           s/op
ShapeSingleShotBenchmark.testParseLegacy                      ss    2  14,687           s/op
ShapeSingleShotBenchmark.testParseLegacyStringInterning       ss    2  25,953           s/op
ShapeSingleShotBenchmark.testParseStringInterning             ss    2  24,526           s/op
ShapeSingleShotBenchmark.testParseStringSelectiveInterning    ss    2  11,911           s/op

So this seems to indicate that interning takes a lot of time.

[skjolber]

This library sometimes implements its own version of String.intern(). Presumably there was a problem with it 10+ years ago, but these days are long gone, so I think we can just use the JVM functionality.

Yes, but then the memory usage is permanent even for numeric strings etc. I think string interning is good as is.

[leonardehrenfried]

Using _ in the field names read like Python. Please don't spread it even more. If you want, you can change all the field names in this class.

[skjolber]

I agree, but still better to have a single convention within the library.

[leonardehrenfried]

Also stop_times.txt is often large. Does it pay off profiling the id interning there? (This can come in a separate PR.)

[leonardehrenfried]

I have a few requests.

[skjolber]

Also stop_times.txt is often large. Does it pay off profiling the id interning there? (This can come in a separate PR.)

AgencyId is just used as a key in StopLocationFieldMappingImpl so don't think there is anything to save on interning it there.

Plain interning seems to save about 80 MB memory and run 1 second (6 vs 7) slower.