Skip to content
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

Avoid using shared_ptr for the linestring's kdtree #54

Open
francocipollone opened this issue Feb 14, 2023 · 1 comment
Open

Avoid using shared_ptr for the linestring's kdtree #54

francocipollone opened this issue Feb 14, 2023 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@francocipollone
Copy link
Contributor

francocipollone commented Feb 14, 2023

See #52 (comment) discussion

Summary

std::shared_ptr<KDTree> kd_tree_;

  • Use unique_ptr instead of shared_ptr
  • This might trigger several changes across maliput_sparse maliput_osm as the linestring will stop being copyable.

To keep in mind(from comments):

Then, it makes sense to have a view-type of the LineString that allows you to access it but not necessarily own the memory. It could be arranged somewhere else. For that to happen, we can change the builder to construct LineStrings at the Segment level, and then organize how we want them to be used. That means, we could derive LaneGeometries from the set of LineStrings or explicitly organize the pair of LineStrings at the moment of construction the LaneGeometry. This does not require a shared_ptr.

@francocipollone
Copy link
Contributor Author

francocipollone commented Feb 15, 2023

Continuing the conversation from #52 (comment)

I think it will play against us any potential parallelization we would like to use.

I don't think it will be a blocker for parallelization.

  • shared_ptrs are thread-safe by implementation.
    • shared_ptrs use atomic increments/decrements of a reference count value.
    • The standard guarantees only one thread will call the delete operator on a shared object
  • data races?
    • Generally speaking, of course there could be data races if the object in the heap is modified from different threads.
    • In our case, the kdtree isn't modified but just read, so R-R operation won't cause any data race. Furthermore, the kdtree implementation doesn't modify its state when nearest-neigbour is queried.

I still believe that using a shared_ptr could be beneficial memory-wise and it will be thread-safe for the LineString.

Let me propose this use case:

  • A highway with 6 lanes.
    • Linestrings defining the lanes are built out a high number of points (>1000). Whether because the definition is dense or because the lanes are too long.
    • Typically for using maliput_sparse's builder we will need 12 linestrings instances (2 linestrings per lane.) to define this segment.
      • 12 kdtree will be maintained in memory for this segment.

With this context, we could make use of the adjacency property and reuse the same linestring, e.g: left linestring of lane 1 == right linestring of lane 2. As the underlying kdtree is under a shared_ptr, this line string will be copyable and the kdtree won't be necessary to be re-created: Better for timing, better for memory usage.

If we want to have this kind of memory management without using shared_ptr we should let the linestring to be owned by other entity (-> segment?) and correctly pass the raw pointer around.
Also, the LineString is a pretty basic entity which is expected to be first instantiated by the parser and at that point a figure of "segment" won't be available until the lanes are identified into segments. So it isn't direct how to group the linestrings.
From a user point of view, the use of LineString class is way less user-friendly with this last approach.

wdyt @agalbachicar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 🆕 Inbox
Development

No branches or pull requests

1 participant