Project-OSRM / osrm-backend

Open Source Routing Machine - C++ backend
http://map.project-osrm.org
BSD 2-Clause "Simplified" License
6.29k stars 3.32k forks source link

Unify shared memory loading code and de-serialization #3899

Closed TheMarex closed 1 month ago

TheMarex commented 7 years ago

3892 already lay the groundwork for this but I wan to spec out the plan in more detail here:

We now have an interface for serialization/deserialization that looks something like this:

serialization::read(FileReader &reader, SomeDataStructue &data_structure);
serialization::write(FileWriter &writer, SomeDataStructue &data_structure);

We want to hide the fact that SomeDataStructure might not own it's backing memory (e.g. by using util::vector_view instead of std::vector) from the de-serialization code.

This is straight forward to do as can be demonstrated here for some classes where I already ported the new model:

This code is starting to look very similar to what we do in ContingiousInternalMemDataFacade:

https://github.com/Project-OSRM/osrm-backend/blob/dce7d5672b5d88d3d4c738ced8ef32da47769885/include/engine/datafacade/contiguous_internalmem_datafacade.hpp#L421-L473

In fact it is the same code save for the additional call to extractor::files::readSegmentData. So there will be a nice opportunity of a refactor:

SegmentDataView makeSegmentDataView(const DataLayout &)
{
         // .... code from data facade goes here ....
}

To fully implement this we need to hit a few milestones:

  1. All file reading/writing must be fully ported to the include/{component}/files.hpp and include/{component}/serialization.hpp schema.
  2. The data facades need to be refactored to put all object initialization into factory functions like above
  3. storage.cpp and the datafacades need to be refactored to use said factory functions

/cc @daniel-j-h @duizendnegen @danpat

duizendnegen commented 7 years ago

:+1:

Do the components that you talk about map directly to files? Will serialization & deserialization logic be contained within the same factory? I see some wins there as well with regards to understandability.

Additionally, it'd be useful to have a factory to obtain the serializer itself - so we can swap capnproto / other serialization protocols / native binary serialization, i.e. serialization::read(FileReader &reader, SomeDataStructue &data_structure); becomes serializationFactory::getSerializer()->read(FileReader &reader, SomeDataStructue &data_structure);. The serialization factory could then either read flags / env. vars / a config file to know which serializer to use.

TheMarex commented 7 years ago

Removing the milestone here, as this is a long-running incremental task. I'll try to hit as many outstanding spots as possible when doing related changes.

github-actions[bot] commented 2 months ago

This issue seems to be stale. It will be closed in 30 days if no further activity occurs.