OpenRailAssociation / osrd

An open source web application for railway infrastructure design, capacity analysis, timetabling and simulation
https://osrd.fr
GNU Lesser General Public License v3.0
419 stars 40 forks source link

Bulk get cached projection #7968

Closed flomonster closed 6 days ago

flomonster commented 1 week ago

Description

The idea is to bulk-get the cached projection to limit the number of queries performed to Redis.

Benchmark

Condition:

The path_projection performance doesn't seem to change (1.2s).

Queries performances:

I think in production, because of queries overhead the difference will be far more important.

The number of Redis queries is reduced by the number of valid train schedule. (2211 -> 1532 queries). Next, I will do the same to retrieve the path and simulation from the cache (but this implies code refactoring).

[!IMPORTANT] There is a limitation with this. I didn't find how to set the expiration time of Redis entry with a bulk command. So now the Redis entry expires after 1 week even if they're used recently.

codecov-commenter commented 1 week ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 2.38095% with 41 lines in your changes missing coverage. Please review.

Project coverage is 28.25%. Comparing base (ec8e87e) to head (f22880b). Report is 1 commits behind head on dev.

Files Patch % Lines
editoast/src/views/v2/train_schedule/projection.rs 4.34% 22 Missing :warning:
editoast/src/redis_utils.rs 0.00% 19 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #7968 +/- ## ============================================ - Coverage 28.26% 28.25% -0.01% Complexity 2075 2075 ============================================ Files 1276 1276 Lines 156178 156197 +19 Branches 3084 3084 ============================================ - Hits 44137 44133 -4 - Misses 110200 110223 +23 Partials 1841 1841 ``` | [Flag](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [core](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `75.03% <ø> (ø)` | | | [editoast](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `71.00% <2.38%> (-0.06%)` | :arrow_down: | | [front](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `9.92% <ø> (ø)` | | | [gateway](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `2.34% <ø> (ø)` | | | [railjson_generator](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `87.49% <ø> (ø)` | | | [tests](https://app.codecov.io/gh/OpenRailAssociation/osrd/pull/7968/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `72.93% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

flomonster commented 1 week ago

There is a limitation with this. I didn't find how to set the expiration time of Redis entry with a bulk command. So now the Redis entry expires after 1 week even if they're used recently.

I found a solution. We can use a transaction or pipe to execute all the commands simultaneously. Example with the CLI:

$ MSET hey 42 hi 43
$ MULTI
$ EXPIRE hey 10
$ EXPIRE hi 10
$ EXEC
flomonster commented 1 week ago

A better solution is to set up an allkeys-lru policy to let Redis handle this automatically.