Split shared memory into updatable and non-updatable section #5002
Conversation
TheMarex
force-pushed
the
feature/updatable_shm
branch
from
76dad11
to
59e2d93
Compare
| @@ -17,4 +17,27 @@ Feature: osrm-datastore command line options: list | |||
| Then it should exit successfully | |||
| When I try to run "osrm-datastore --list" | |||
| Then it should exit successfully | |||
| And stdout should contain "test_dataset_42/data" | |||
| And stdout should contain "test_dataset_42/static" | |||
There was a problem hiding this comment.
Maybe we can add And stdout should contain "test_dataset_42/updatable" too
| @@ -43,18 +43,24 @@ class DataWatchdogImpl<AlgorithmT, datafacade::ContiguousInternalMemoryDataFacad | |||
| boost::interprocess::scoped_lock<mutex_type> current_region_lock(barrier.get_mutex()); | |||
|
|
|||
| auto &shared_register = barrier.data(); | |||
| auto region_id = shared_register.Find(dataset_name + "/data"); | |||
| if (region_id == storage::SharedRegionRegister::INVALID_REGION_ID) | |||
| auto static_region_id = shared_register.Find(dataset_name + "/static"); | |||
There was a problem hiding this comment.
we could replace auto with storage::SharedRegionRegister::RegionID 🤷♀️
code style nitpick and up to you whether you want to pick it up or not 😂
There was a problem hiding this comment.
I usually prefer the short version, since it makes changes to the code easier. For example, when renaming the type storage::SharedRegionRegister::RegionID it should not break the code in too many places.
I found that in most cases it is good practice to write your code type-safe but also type-agnostic, meaning the type information is used by the compiler to find errors, but when reading the code you don't really have to care about the types that much. I think the short rule was: "auto whenever possible".
There was a problem hiding this comment.
yea. i can live with both. they have their advantages and disadvantages 👍
There was a problem hiding this comment.
@TheMarex sounds like Swift :-)
TheMarex
force-pushed
the
feature/updatable_shm
branch
from
c1f39a8
to
fb1540e
Compare
src/storage/storage.cpp
Outdated
| std::unordered_map<std::string, contractor::ContractedMetricView> metrics = { | ||
| {metric_name, std::move(contracted_metric)}}; | ||
|
|
||
| std::uint32_t graph_connectivity_checksum = 0; | ||
| contractor::files::readGraph( | ||
| config.GetPath(".osrm.hsgr"), metrics, graph_connectivity_checksum); | ||
|
|
||
| if (turns_connectivity_checksum != graph_connectivity_checksum) |
There was a problem hiding this comment.
is it ok to remove this check? is it impossible to keep this check because .hsgr is now part of updatable?
There was a problem hiding this comment.
Good point, I forgot about removing this. Thinking about this, there is probably a way to retain this. 👍
src/storage/storage.cpp
Outdated
| std::uint32_t graph_connectivity_checksum = 0; | ||
| partitioner::files::readGraph( | ||
| config.GetPath(".osrm.mldgr"), graph_view, graph_connectivity_checksum); | ||
|
|
||
| if (turns_connectivity_checksum != graph_connectivity_checksum) |
TheMarex
force-pushed
the
feature/updatable_shm
branch
from
3beec58
to
8a7e392
Compare
|
I investigated the coverage failures and it seems like the alternatives acting up. :-/ |
Issue
This is a simple version of #4873 that still works on per-file granularity. To make this more effective we need to split the weights from the MLD graph and refactor the way we update speeds (to work on a graph not an edge list).
Preliminary measurements on California show that this reduces peak memory usage by 25%.
Tasklist
Requirements / Relations
Based on #4982