Clean libosrm from concepts and dependencies external to library's design

[daniel-j-h]

In the strict sense libosrm does not need to know about responses being encoded as JSON or hints being encoded in Base64.

JSON is only needed for the HTTP use-case we have e.g. with node-osrm. Why not build node-osrm completely on top of libosrm? A libosrm user neither needs JSON nor the variant dependency it comes with.

The same goes for the hint. The requirement for Base64 encoding comes from the HTTP use-case and is not inherent in libosrm. Using libosrm I am able to store a Hint-typed object just fine.

Feel free to discuss.

[TheMarex]

JSON is only needed for the HTTP use-case we have e.g. with node-osrm. Why not build node-osrm completely on top of libosrm?

I don't know what you are getting here on. We need a way to represent the response. We either come up with a C++ structure that encapsulates our response, but in that case we then need to write an additional translation layer that transforms the C++ API structutre to the JSON API structure. This is unneeded overhead and we optimize for our use case - direct mapping to JS structures.

The requirement for Base64 encoding comes from the HTTP use-case and is not inherent in libosrm.

Yes this is why RouteParameters contains Hint objects and not strings. If you want o move the serialization/deserialization outside of the hint.hpp header feel free to move it to the server completely.

[daniel-j-h]

Bumping this issue in light of our recent findings: assembling the JSON object / variant is heavy on the allocator. Quick fix would be to try a pool allocator or link in e.g. jemalloc if it's found on the user's system. What are our long-term plans for fixing this?

[TheMarex]

@daniel-j-h I did an experiment with a custom Array type for coordinates (e.g. the geojson stuff which is a real bottle neck) that was basically std::array<2, json::Value> and didn't observe dramatic speedups. Which makes me think the actual heap allocation is not the main problem here. Though trying jemalloc is worth a try.

Long term I would like to explore the option of dropping the internal JSON for libosrm and expose the internal RouteStep, RouteLeg etc objects. Main motivation would be the possibility of exposing std::vector<Coordinate> for the geometry and not needing to handle format encoding in OSRM. This has some consequences on where certain code needs to live.

1. JSON serialization would need to live in node-osrm. If we want to keep the C++ version of osrm-routed, it needs to be duplicated there.
2. Geometry format encoding needs to happen outside of libosrm. node-osrm is an unlikely place, we would push this up one more level to the application. osrm-routed would need dedicated code for this as well.

Since this would mean OSRM 6 I want to be sure we have a very good idea what this would mean on the performance side and impact of code reuse.

[TheMarex]

I think we should explore this even before breaking the API for this. Once https://github.com/Project-OSRM/osrm-backend/pull/3768 lands it is easier to refactor where something is converted to JSON/V8 or just plain structs.

This would basically need an abstraction layer that works on the plain C++ struct version and exports the same format that we are currently exposing.

• [ ] engine/api/* should not need to know about JSON in general (RouteAPI, TableAPI, ... )
• [ ] What is now engine/api/json_factory.hpp should be refactored into a C++ object -> JSON objects translation layer
• [ ] We should add a similar translation layer for doing C++ object -> V8 directly (instead of C++ -> JSON -> V8)
• [ ] All libosrm internal interfaces only use the C++ API struct versions.

• [ ] libosrm's public interface provides additional overloads:

  class OSRM
  {
       Status Route(const RouteParameters &params, json::Object &result);
       Status Route(const RouteParameters &params, RouteResult &result);
  }

• [ ] Deprecate JSON based interface
• [ ] Remove JSON based interface in 6.0

[github-actions[bot]]

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