Skip to content

Make turn restrictions faster and thread-safe by moving them into StreetEdge#3899

Merged
leonardehrenfried merged 18 commits into
opentripplanner:dev-2.xfrom
ibi-group:turn-restrictions-into-edge
Feb 22, 2022
Merged

Make turn restrictions faster and thread-safe by moving them into StreetEdge#3899
leonardehrenfried merged 18 commits into
opentripplanner:dev-2.xfrom
ibi-group:turn-restrictions-into-edge

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Feb 11, 2022

Copy link
Copy Markdown
Member

Summary

It moves the turn restrictions form a global map to the individual StreetEdge instances.

This has several advantages:

  • It makes it easier to reason about
  • Profiling has shown that the map access is comparatively slow
  • It's easier to make a method on StreetEdge thread-safe than one on Graph

Thread safety

This is actually a bit trickier than one might suspect since we have the following constraints:

  • not setting the turn restrictions to null (as this would make access be synchronized)
  • not creating a separate empty set for every street edge
  • synchronized or concurrent modification
  • fast access
  • being able to serialize the turn restriction collection

For these reasons I've chosen the following design:

  • a global instance of an empty set of turn restrictions that is shared between all instances that have none
  • synchronized modification with copy-on-write semantics
  • fast access without extra copying or allocations by making the collection immutable after modification

Issue

Relates to #3884 but not confirmed that it solves the issue.

Unit tests

Unit tests adjusted

Code style

yes

Documentation

Plenty of Javadoc added.

@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner February 11, 2022 09:56
@leonardehrenfried
leonardehrenfried force-pushed the turn-restrictions-into-edge branch from a3c525f to c3fe836 Compare February 11, 2022 10:13
@hannesj
hannesj self-requested a review February 11, 2022 12:03
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
@t2gran t2gran added this to the 2.1 milestone Feb 11, 2022
@t2gran t2gran added !Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes. labels Feb 11, 2022
Comment thread src/main/java/org/opentripplanner/common/TurnRestriction.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java
@leonardehrenfried
leonardehrenfried force-pushed the turn-restrictions-into-edge branch from 4d2e0f0 to 0159fe5 Compare February 14, 2022 10:03
@leonardehrenfried
leonardehrenfried force-pushed the turn-restrictions-into-edge branch from 0159fe5 to 5831ccc Compare February 14, 2022 10:28
Comment thread src/main/java/org/opentripplanner/routing/graph/SerializedGraphObject.java Outdated

@hannesj hannesj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to reduce the time spent in StreetRouter from 19.5 ms to 18.0 ms in the SpeedTest.

kryo_set_serialization.diff can be used to fix the serialization issue, with the java.util.ImmutableCollections Set

@leonardehrenfried
leonardehrenfried force-pushed the turn-restrictions-into-edge branch from 4be5bd3 to c955e7e Compare February 22, 2022 08:21
Comment on lines +11 to 16
public final TurnRestrictionType type;
public final StreetEdge from;
public final StreetEdge to;
public final RepeatingTimePeriod time;
public final TraverseModeSet modes;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice

hannesj
hannesj previously approved these changes Feb 22, 2022
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java Outdated
@t2gran

t2gran commented Feb 22, 2022

Copy link
Copy Markdown
Member

There is probably a long discussion on the use of Set vs List here. And I would agree that we sometimes want to use Set just to communicate the "unique"-ness of the elements. But, Set have a bigger overhead to then (maybe not for 1 and 2 elements), but at least mentally - so wen memory footprint and performance is high priority I prefer to use list, and just make sure the construction of them enforce the "unique"-ness constraint. Also, in this case there is no need to expose the collection type outside the StreetEdge.

@leonardehrenfried
leonardehrenfried force-pushed the turn-restrictions-into-edge branch from 4e40034 to 63478d9 Compare February 22, 2022 15:47
@leonardehrenfried

Copy link
Copy Markdown
Member Author

There is probably a long discussion on the use of Set vs List here. And I would agree that we sometimes want to use Set just to communicate the "unique"-ness of the elements. But, Set have a bigger overhead to then (maybe not for 1 and 2 elements), but at least mentally - so wen memory footprint and performance is high priority I prefer to use list, and just make sure the construction of them enforce the "unique"-ness constraint. Also, in this case there is no need to expose the collection type outside the StreetEdge.

I'm happy to go along with this suggestion.

@leonardehrenfried
leonardehrenfried merged commit 04c9ddf into opentripplanner:dev-2.x Feb 22, 2022
@leonardehrenfried
leonardehrenfried deleted the turn-restrictions-into-edge branch February 22, 2022 16:02
t2gran pushed a commit that referenced this pull request Feb 22, 2022
@leonardehrenfried

Copy link
Copy Markdown
Member Author

🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants