-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicated dirty timetables #6153
Fix duplicated dirty timetables #6153
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6153 +/- ##
=============================================
- Coverage 69.91% 69.90% -0.01%
+ Complexity 17701 17698 -3
=============================================
Files 1996 1996
Lines 75305 75306 +1
Branches 7705 7705
=============================================
- Hits 52646 52640 -6
- Misses 19989 19994 +5
- Partials 2670 2672 +2 ☔ View full report in Codecov by Sentry. |
* A real-time timetable overrides the scheduled timetable of a TripPattern for only a single | ||
* service date. There can be only one overriding timetable per TripPattern and per service date. | ||
* This is enforced by indexing the map with a pair (TripPattern, service date). | ||
* This field will be set to null when the TimetableSnapshot becomes read-only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I get it. What is being set to null? I don't see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last sentence was copied from the original Javadoc, and you're right, this was not correct: the collection is cleared, not set to null.
Javadoc fixed.
|
||
TransitLayerUpdater transitLayer = new TransitLayerUpdater(null) { | ||
@Override | ||
public void update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a check that this method is called at all? A simple (atomic) boolean is enough for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Collection<Timetable> updatedTimetables, | ||
Map<TripPattern, SortedSet<Timetable>> timetables | ||
) { | ||
assertEquals(1, updatedTimetables.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using truth here would give a nicer error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Besides fixing the memory leak this also appears to improve correctness, isn't it? What happened when those duplicate Timetables ended up in Raptor? |
This is a serious bug that has probably other side effects. |
The mkdocs installation is fixed by #6154 which is already merged. A merge/rebase should fix the issue. |
@optionsome @vesameskanen Can any of you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to timetables of unused trip patterns? For example, at some point stop is skipped but then it isn't again. I know I've worked around that stuff few months ago but don't know how much things have changed since.
I'll approve this since the memory leak potentially caused by unused patterns should be quite small compared to the issue this fixes.
FWIW, this build looks a lot better. Will send another update in the morning, after running my ad-hoc stress test overnight. But graph has been running for a few hours, and the stress test running over 1 hour now, and things look good so far. |
The logic for reverting trips to their original trip pattern uses the same method (swapTimetable) that is fixed in this PR, which means that the memory leak is also fixed in this case. OpenTripPlanner/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java Line 459 in f4bfedf
|
If I understand correctly, the memory leak is prevented by the old timetables for TripPattern+service date getting overwritten by new entries for the same pattern and service date. However, for realtime added trip patterns, if a trip reverts back to using scheduled trip pattern, then the realtime added trip pattern and its timetables might still exists in the dirty timetables map because nothing is writing on top of the old entry. |
I think the best way to prove or disprove this theory would be to write a unit test. |
I think I might have added some tests earlier that check that the old timetables don't at least leak into use. The memory leak potentially caused by these is much smaller than the one caused by updates to old trips left by streaming updaters. Probably the biggest concern is just consistency when there could be multiple versions of realtime timetables for the same trip. |
Summary
As detailed in #6135, a memory leak has been identified in the
TransitLayer
.This bug was introduced in #6017.
The set of dirty timetables in
TimetableSnapshot
(that is: the set of timetables currently being modified and to be added to the next snapshot) should contain only one modified timetable per trip pattern and per service date.Prior to #6017, if a timetable was modified several times when building the next snapshot, the same
Timetable
instance would be reused and updated, thus guaranteeing uniqueness.In #6017,
Timetable
is immutable and the previous Timetable instance cannot be modified : it must be swapped with a newTimetable
instance that integrates the latest changes.However the code fails to remove the previously modified timetable before adding the new one and the
Set
that holds the dirty timetables does not enforce uniqueness (Timetable
does not have an id and does not implement equals()).A simple fix would be to keep the existing data structure (
Set
) and explicitly remove the old instance before adding the new one.Here a more reliable fix is proposed, where uniqueness is enforced by storing the dirty timetables in a map indexed by (
TripPattern
, service date).Issue
Closes #6135
Unit tests
Added unit test.
Documentation
No