Iox #859 implement prefix tree

[MatthiasKillat]

Pre-Review Checklist for the PR Author

1. Code follows the coding style of CONTRIBUTING.md
2. Tests follow the best practice for testing
3. Changelog updated in the unreleased section including API breaking changes
4. Branch follows the naming format (iox-#123-this-is-a-branch)
5. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit messages are signed (git commit -s)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
6. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
7. Relevant issues are linked
8. Add sensible notes for the reviewer
9. All checks have passed (except task-list-completed)
10. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
  • Each unit test case has a unique UUID
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

Review notes

1. introduced a TypedAllocator as a building block for the prefix tree (but is more generally useful), should be reviewed first
2. introduced a PrefixTree, how it works may require some more explanation but I am hesitant to create a design document since it is a known data structure (with additional complications due to our static memory restrictions)
3. objects of both classes are relocatable and I am working on writing a general recipe (*.md) to obtain those kind of structures in a formulaic way, this is not included here

References

• Closes Implement a prefix tree for fast string search #859

[MatthiasKillat]

Interface is suitable for currently intended use cases (string search in service registry) but can of course be extended later as needed. Returning values can be done in addition, but should not be the default.

The important part is to support efficient and (somewhat) convenient insertion, removal and look-up (for reading and modification) of values.

Since it has multi-map semantics (required for our use case), I opted against operator[] for value access (there can be more values per key and we need to support this).

[elfenpiff]

I would vote here to use as return value a cxx::vector<std::reference_wrapper<Value>, Capacity>.

In our safety environment you force a nullptr check onto the user since in theory a pointer could be always null but a reference is always non null. And this nullptr check can be expensive if one iterates over a large vector.

Using this in a vector is also straight forward:

myVector.emplace_back(std::ref(myValue));

[elfenpiff]

Oh and I saw that you are breaking the const correctness here. A user could acquire such a vector and is able to change the underlying values despite that this method is const and the tree should not be modifiable in such a scope. When further thinking about this, this can be even more hazardous.

Lets assume someone acquires such a result and is moving this result into a separate thread and now another thread is changing the prefix tree. One can run into race conditions extremely easy here since one is not aware that the return value contains references to the underlying values which can change.

With this in mind I would really prefer a copy here which would be just safer. But I have to admit that this would be quiet inefficient here I would propose that you implement the callback find interface idea you mentioned earlier. Something like:

void find(const Key & key, const cxx::function_ref<void(const Value& value)> &callback) const noexcept;

and the callback is called for every fitting key.

[elBoberido]

Could you please create a small design document for this feature

[MatthiasKillat]

@elBoberido Yes, I will create a design document. It shall contain the requirements and high level ideas but I will avoid code there. I usually distinguish between abstract/high-level design for the ideas and concepts and detailed design for e.g. interfaces and implementation details (should be used sparingly since they are subject to change).

I will create a high-level design document.

Note that I am first and foremost interested in whether we can live with the public interface for now. The functionality will not change much I think, but the interface is up to discussion (to a degree, we will not get the "perfect" now).

[elfenpiff]

Reviewed only until line 105 in prefix_tree.inl (insert).

[elfenpiff]

I would remove the bool return value here. If you call remove the contract states that in either case the key is removed independent of the return value.

But when you read something like

if ( myTree.remove("someKey") == false )
  // ...

One may easily interpret this as some kind of error and implements some excessive error handling around it - and this has to be tested etc. It is not like it already happened before in the service registry.

If one would really know if something is present one could always use find or maybe it even makes sense to implement a method like bool hasKey(const Key &key)

[elfenpiff]

This is a pretty nice idea!

[elfenpiff]

I would vote here to use as return value a cxx::vector<std::reference_wrapper<Value>, Capacity>.

In our safety environment you force a nullptr check onto the user since in theory a pointer could be always null but a reference is always non null. And this nullptr check can be expensive if one iterates over a large vector.

Using this in a vector is also straight forward:

myVector.emplace_back(std::ref(myValue));

[elfenpiff]

Like I mentioned above, please use cxx::vector<std::reference_wrapper<Value>, Capacity> as return value.

[elfenpiff]

Oh and I saw that you are breaking the const correctness here. A user could acquire such a vector and is able to change the underlying values despite that this method is const and the tree should not be modifiable in such a scope. When further thinking about this, this can be even more hazardous.

Lets assume someone acquires such a result and is moving this result into a separate thread and now another thread is changing the prefix tree. One can run into race conditions extremely easy here since one is not aware that the return value contains references to the underlying values which can change.

With this in mind I would really prefer a copy here which would be just safer. But I have to admit that this would be quiet inefficient here I would propose that you implement the callback find interface idea you mentioned earlier. Something like:

void find(const Key & key, const cxx::function_ref<void(const Value& value)> &callback) const noexcept;

and the callback is called for every fitting key.

[elfenpiff]

I think you can adjust the API here so that you use const Key & key instead of const char* letters, uint32_t length. In all your uses you get the letters and length always from the key and then put them into this function.

Furthermore I would introduce a short struct called something like

struct NodeProperties { // I know, the name is not perfect ;)
  Node& node;
  uint32_t prefixLength;
};

and return it here. This would make the API more readable and we avoid discussions in the code like uninitialized variables. The final version could then look like:

NodeProperties findPrefix(const Key & key) const noexcept;

[elfenpiff]

Please use lowerCamelCase. When you activate clang-tidy in your IDE and use our .clang-tidy rules you will get a hint directly in the IDE.

[elfenpiff]

The method has the same name as the method in the public API. This is a little bit confusing and since they are performing different task please rename one of them.

[elfenpiff]

After reviewing this method I do not really see why we have to return a boolean to the user at all. And strictly speaking we are also somehow lying to them, the key still exists but does not contain any data.

[elfenpiff]

I think recursions are forbidden in code we want to certify.

[elBoberido]

@MatthiasKillat what would be extremely helpful is a class diagram and a diagram similar to what you had in your C++ talk, showing the interaction of the Nodes and how a simple lookup will be performed. This would make it much easier to build a mental model for the code to review

[MatthiasKillat]

@elBoberido Class diagram makes sense. Not sure what kind of diagram you had in mind for he lookup. Since it relies on proper positioning it may be hard to generate with PlantUML. Sketching it in ascii will work of course. I am on it, please stop reviewing until then.

[elBoberido]

@MatthiasKillat I meant sketching in ascii. Using PlantUML for this would be horrible

[MatthiasKillat]

@elBoberido added a design document which hopefully explains the intended behavior, use case and design decisions and helps building the mental model needed to understand the underlying algorithms.

I avoided code there and have no class diagram yet. Class interfaces and interaction are of little concern here, the node structure is. Diagrams can be added though, if needed. Boils down to the interfaces and some "has_a" relations though, do we want that?

[MatthiasKillat]

I could not find find a good source for this, but this is the original paper. It is also not terribly important as it is concerned with the pointer structure of the nodes which I explain below.

[elBoberido]

@MatthiasKillat I'm not sure if we need a simple class diagram. Maybe it is necessary for certification.

[MatthiasKillat]

Maybe also in the 1000s, but 10000s are probably too much for brute force search (depending on application). Search time in the tree is basically constant if the key length is limited.

[elBoberido]

@MatthiasKillat you once mentioned you wanted to split this up into multiple PR with preparation work being merged first. Is this still your plan or are you waiting for review?

[MatthiasKillat]

@elBoberido Yes, I want to split at least the memory allocator part as I would like to have it as a clean abstraction for the future (could also be used in posh, maybe). I need to think how to continue with the tree, there is no immediate need. Ideally we would have some of thse data structures for shared memory usage and I would also like to benchmark them. But this has lower priority.

[MatthiasKillat]

@elBoberido I still have the plan to move the allocator from this PR to another PR and then probably close this one. However, I had no time yet due to other priorities. It is also not urgent.

We can close this one regardless. Still, I consider part of the work important for future data types (just not now with current priorities).

[MatthiasKillat]

Currently it is not needed, has diverged considerably and is relatively complex to review. The basic ideas will be revisited for strategies to create tree structures for iceoryx and as shared memory types.

The data structure could be useful for string indexing internally, and more specifically in a service discovery refactoring.