-
Notifications
You must be signed in to change notification settings - Fork 45
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
core: stdcm: optimize incremental conflict detection #9980
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #9980 +/- ##
==========================================
- Coverage 81.48% 81.47% -0.02%
==========================================
Files 1058 1058
Lines 104270 104270
Branches 722 722
==========================================
- Hits 84969 84956 -13
- Misses 19260 19273 +13
Partials 41 41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7e3019f
to
3287ab9
Compare
9883462
to
3287ab9
Compare
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.
Thanks for this works, found the new code quite easy to read indeed 🙏
(some of my suggestions may tend to old code intricacy...)
Didn't review the old code and what might be missing from it.
As I understand it, we just don't check the routing requirements since STDCM v2, and this PR removes the check from v1. Which is fine by me (trusting you on a late non-incremental check for routing conflicts 🙈).
core/src/test/kotlin/fr/sncf/osrd/conflicts/IncrementalConflictDetectorTests.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Outdated
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Show resolved
Hide resolved
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.
Removing the routingRequirements
+ higherEntry
really simplifies everything. Maybe we'll be able to do things more easily with this structure as well once/if we have to add the routing requirements back (it was really bloated with the previous way it was coded).
I'll let you deal with the other comments, but this looks pretty decent already.
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Show resolved
Hide resolved
core/src/main/java/fr/sncf/osrd/conflicts/IncrementalConflictDetector.kt
Outdated
Show resolved
Hide resolved
04e7379
to
d9a0f84
Compare
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.
Improved perf and much clearer to me too, thanks for all this, really helps to dive in this part 🙏
66bad79
to
77d5703
Compare
Not used for now and duplicates code, but it enables optimizations Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>
Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>
Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>
77d5703
to
1493c83
Compare
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.
Gj!
Fix #9801
Helps with #10034 without entirely fixing it
Possible review by commit (though it's possible to just ignore the previous version and read the new one on its own):
I haven't run a proper benchmark as it requires a pretty specific setup (thousands of trains in a realistic timetable), but I do have some metrics:
down to 2-5%This PR also reduces code complexity by quite a lot, and may fix some of the "mismatch" crashes.
edit: hold on, the new version still takes up a lot of time when it timeout (a lot more than 2-5% execution time). On one massive request, the time we spend in the conflict detection methods goes from 71% to 45% execution time: it's much better but still quite a lot. edit2: partially fixed by #10126.