SuffixTree | Optimized implementation of suffix tree | Computer Vision library

Issue Origin

After having a glorious battle that lasted two days and three nights (and amazing ideas and thoughts from the comments) I've finally managed to fix this issue!

I'd like to post an answer for anybody running into similar issues where the string.Substring(i, j) function is not an acceptable solution to get the substring of a string because the string is either too large and you can't afford the copying done by string.Substring(i, j) (it has to make a copy because C# strings are immutable, no way around it) or the string.Substring(i, j) is being called a huge number of times over the same string (like in my nested for loops) giving the garbage collector a hard time, or as in my case both!

Attempts

I've tried many suggested things such as the StringBuilder, Streams, unmanaged memory allocation using Intptr and Marshal within the unsafe{} block and even creating an IEnumerable and yield return the characters by reference within the given positions. All of these attempts failed ultimatively because some form of joining of the data had to be done as there was no easy way for me to traverse my tree character by character without jeopardizing performance. If only there was a way to span over multiple memory addresses within an array at once like you would be able to in C++ with some pointer arithmetic.. except there is.. (credits to @Ivan Stoev's comment)

The Solution

The solution was using System.ReadOnlySpan (couldn't be System.Span due to strings being immutable) which, among other things, allows us to read sub arrays of memory addresses within an existing array without creating copies.

This piece of the code posted:

Your handling of deepest_shared_edge is wrong. First, the allocation you do at the start of the function is a memory leak, since you never free the memory. Secondly, the result of the recursive call is ignored, so whatever deepest edge it finds is lost (although you update the depth, you don't keep track of the deepest edge).

To fix this, you should either pass deepest_shared_edge as a reference parameter (like you do for longest), or you can initialize it to nullptr, then check the return from your recursive call for nullptr and update it appropriately.

In C#, a System.String includes its Length, so you can get the length in constant time. In C++, a std::string also includes its size, so it is also available in constant time.

However, you aren’t using C++ std::string (which you should be, for a good translation of the algorithm); you’re using a C-style null-terminated char array. That char* literally means “pointer to char”, and just tells you where the first character of the string is. The strlen function looks at each char from the one pointed to forward, until it finds a null character '\0' (not to be confused with a null pointer); this is expensive, and you do it in each iteration of your loop in insertSuffix. That probably accounts for at least a reasonable fraction of your slowdown.

When doing C++, if you find yourself working with raw pointers (any type involving a *), you should always wonder if there’s a simpler way. Sometimes the answer is “no”, but often it’s “yes” (and that’s getting more common as the language evolves). For example, consider your struct node and node* root. Both use node pointers, but in both cases you should have used node directly because there is no need to have that indirection (in the case of node, some amount of indirection is necessary so you don’t have each node containing another node ad infinitum, but that’s provided by the std::unordered_map).

A couple other tips:

• In C++ you often don’t want to do any work in the body of a constructor, but instead use initialization lists.
• When you don’t want to copy something you pass as a parameter, you should make the parameter a reference; instead of changing insertSuffix to take a std::string as the first parameter, make it take std::string const&; similarly, contains should take a std::string const&. Better yet, since insertSuffix can see the text member, it doesn’t need to take that first parameter at all and can just use from.
• C++ supports a foreach-like construct, which you should probably prefer to a standard for loop when iterating over a string’s characters.
• If you’re using the newest not-technically-finalized-but-close-enough version of C++, C++17, you should use std::string_view instead of std::string whenever you just want a look at a string, and don’t need to change it or keep a reference to it around. This would be useful for contains, and since you want to make a local copy in the text member, even for the constructor; it would not be useful in the text member itself, because the object being viewed might be temporary. Lifetime can sometimes be tricky in C++, though, and until you get the hang of it you might just want to use std::string to be on the safe side.
• Since node isn’t useful outside of the concept of suffixTree, it should probably be inside it, like in the C# version. As a deviation from the C# version, you might want to make the type node and the data members root and text into private instead of public members.