Fix #98 - Reuse Calendar for creating ServiceDate in WSFBlockResolutionStrategy

[skjolber]

Summary:

Reuse Calendar in WSFBlockResolutionStrategy

Expected behavior:

Constructs ServiceDate using another constructor.

Please make sure these boxes are checked before submitting your pull request - thanks!

• [ x] Run the unit tests with mvn test to make sure you didn't break anything
• [ x] Format the title like "Fix #<issue_number> - short description of fix and changes"
• [x ] Linked all relevant issues

[CLAassistant]

All committers have signed the CLA.

[skjolber]

@barbeau any hope for merging this?

[barbeau]

@skjolber Thanks for contributing this! I think so, but I'm not the best one to review this. I'm adding @sheldonabrown and @sdjacobs to the thread to get their reviews for possible merge.

[sdjacobs]

@skjolber, could you give us some more info on why you made this change? To me, it looks like it's essentially nonfunctional - it doesn't break anything, but I'm not sure what it adds. The main performance issue in this code is that it makes tons and tons of API calls to the ferry schedule API; I really can't imagine Calendar object creation is a bottleneck. Is it?

Also, just out of curiosity, why are you in this part of the code? I thought this was a Seattle-specific issue. Are there ferry services in Norway that use the same API?

[skjolber]

This is a performance improvement. Creating calendars appeared to be called relatively frequently / costly when using a profiler. I can't find the exact results, but I believe the use-case was search.

OpenTripPlanner uses an onebusaway dependency so that is the relation.

[barbeau]

@sdjacobs I think the optimization is here - if you look at private long ts(XMLGregorianCalendar xgc) {...} method, within that method it instantiates a GregorianCalendar to get the time, but then throws it away and just returns a long. Then, in resolve() another Calendar object is instantiated again. This cuts out the instatiation in resolve() by passing in an existing instance of Calendar into resolve().

@skjolber Did I get that right?

[skjolber]

@barbeau Yes.

[sdjacobs]

OTP only uses the onebusaway-gtfs submodule, which is the GTFS model classes.

WSFBlockResolutionStrategy is a strategy for onebusaway-gtfs-transformer, a different submodule which is used to apply defined transformations to a GTFS feed. More information on usage here. In particular, this strategy queries the Washington State Ferry API for schedule information, in order to add blocks to the GTFS, as the official provided WSF GTFS doesn't include blocks.

I don't think this change will provide a performance improvement, since the performance of this strategy is totally dominated by the latency of the thousands of requests it makes to the WSF API, rather than Calendar creation. (It certainly won't affect OTP's performance, since OTP doesn't use this submodule.)

I don't have a problem with the change per se, I just want to make sure everyone's clear on what it does (or does not do).

[skjolber]

@sdjacobs I agree this is just a small step in improving the performance. The overall picture seems to be that a lot of garbage is created, triggering (costly) garbage collection relatively often. This calendar reuse is just a low-hanging fruit.

[skjolber]

Ah, there was also an issue with some more details: #98

[sheldonabrown]

To reiterate, this pull request almost certainly has nothing to do with the performance/memory issues you are experiencing. This code path is very specific to one integration strategy that you would not be using (please tell me if that's not the case).

As Simon said, the effort is welcome, but this change will not have the effect you intend. In fact, it should have no affect since it lives outside the typical code path.

[skjolber]

Look, I cant really remember / locate the exact benchmark and code path, so I suggest reviewing this PR as a performance improvement as-is.

[sheldonabrown]

Yes, I totally appreciate that, and we will likely accept the pull request. But we need to be clear it will have no affect on anything you are likely doing. And it will have no affect on the actual strategy as it is I/O bound with respect to webservice latency. So that is to say this PR represents a "non-functional change"

[skjolber]

Ok, thank you for pointing that out again. I'll make sure to describe issues / PRs better in the future.