Skip to content
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

Make TimetableSnapshot state final #5968

Merged

Conversation

vpaturet
Copy link
Contributor

Summary

This PR refactors the TimetableSnapshot class to make its shared state final.
The intent is:

  1. making clear that the TimetableSnapshot fields are immutable after publication.
  2. preparing further refactoring steps where the snapshot could be published without using a lock.
    According to the Java Memory Model (https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5), final fields provide additional protection against re-ordering and can be used to implement thread-safe immutable objects without synchronization.

Note: the field TimetableSnapshot.dirty remains mutable, but is not used by reader threads.

Issue

No

Unit tests

No

Documentation

No

@vpaturet vpaturet added Technical Debt Real-Time Update The issue/PR is related to RealTime updates Skip Changelog labels Jul 15, 2024
@vpaturet vpaturet self-assigned this Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.63%. Comparing base (8de9f88) to head (b508c79).
Report is 2 commits behind head on dev-2.x.

Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5968   +/-   ##
==========================================
  Coverage      69.63%   69.63%           
- Complexity     17129    17132    +3     
==========================================
  Files           1937     1937           
  Lines          73742    73744    +2     
  Branches        7546     7546           
==========================================
+ Hits           51351    51353    +2     
- Misses         19754    19755    +1     
+ Partials        2637     2636    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vpaturet vpaturet marked this pull request as ready for review July 15, 2024 07:00
@vpaturet vpaturet requested a review from a team as a code owner July 15, 2024 07:00
Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

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

Looks good. Although making the fields final doesn't prevent changing the contents of the collections, it expresses the intent and moves in the right direction.

The that setting all the final fields in the constructor provides additional reordering/atomicity guarantees is important. If it's not mentioned in the javadoc here, it should be mentioned at points where the object references are handed off between threads. If only to ensure someone doesn't inadvertently lose this characteristic in the future.

@@ -111,20 +111,36 @@ public class TimetableSnapshot {
* more than once.
* TODO RT_AB: More general handling of all realtime indexes outside primary data structures.
*/
private SetMultimap<StopLocation, TripPattern> patternsForStop = HashMultimap.create();
private final SetMultimap<StopLocation, TripPattern> patternsForStop;
Copy link
Member

Choose a reason for hiding this comment

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

@vpaturet didn't you say earlier that this field was the only one that was not final? Looks like it is indeed final, so this class is essentially a record. Not that I'm advocating changing it to one, just pointing out this detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was referring to this field:
private boolean dirty
which needs to be updated during the lifecycle of the buffer snapshot, but is not used in the published snapshot.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant to put the comment on private final boolean readOnly; but your comment still applies: the non-final field is the one right below it (dirty) which I for some reason didn't notice.

@vpaturet
Copy link
Contributor Author

Looks good. Although making the fields final doesn't prevent changing the contents of the collections, it expresses the intent and moves in the right direction.

The that setting all the final fields in the constructor provides additional reordering/atomicity guarantees is important. If it's not mentioned in the javadoc here, it should be mentioned at points where the object references are handed off between threads. If only to ensure someone doesn't inadvertently lose this characteristic in the future.

I will merge this PR as is and add Javadoc about safe publication of final fields in a follow-up PR.

@vpaturet vpaturet merged commit 5917ddf into opentripplanner:dev-2.x Jul 16, 2024
5 checks passed
@vpaturet vpaturet deleted the make_timetable_snapshot_state_final branch July 16, 2024 15:35
@t2gran t2gran added this to the 2.6 (next release) milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Real-Time Update The issue/PR is related to RealTime updates Skip Changelog Technical Debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants