toposort use iterator to avoid stackoverflow

[tang-hi]

Description

In Issue #12284, I observed that Lucene limits the recursion level in the topoSortStatesRecurse method to avoid a StackOverflow error during automaton topological sorting. I propose we could use an iterative approach instead of recursion.
I've implemented an iterative version as shown below.

private static int topoSortStatesRecurse(
      Automaton a, BitSet visited, int[] states) {
      
    Stack<Integer> stack = new Stack<>();
    stack.push(0); // Assuming that the initial state is 0.
    int upto = 0;
    Transition t = new Transition();

    while (!stack.empty()) {
        int state = stack.pop();

        int count = a.initTransition(state, t);
        for (int i = 0; i < count; i++) {
            a.getNextTransition(t);
            if (!visited.get(t.dest)) {
                visited.set(t.dest);
                stack.push(t.dest);
            }
        }
        states[upto] = state;
        upto++;
    }
    return upto;
}

However, I noticed that the test TestAutomaton.java depends on the order in which recursive calls are made or items are added to and removed from the stack. To maintain the same order as the recursive version in the iterative approach, so I've used a particular technique in the pull request.

To further improve this change, here are my plans:

1. Remove the trick and modify the test code accordingly.
2. Perhaps we should check if the automaton contains cycles, and throw an IllegalArgumentException if it does?
3. Should we continue to limit the size of the Automaton?
   I welcome any suggestions or feedback on this approach.

[mikemccand]

Thank you @tang-hi! We have tried to build iterative versions of this in the past, but it was conterversial because it was so much more complicated than the existing recursive version. But your iterative solution looks very simple!

However, I noticed that the test TestAutomaton.java depends on the order in which recursive calls are made or items are added to and removed from the stack.

The test really should not be relying on such an implementation detail? By definition, toposort might have many valid outputs for the same input graph. The test should only assert that the returned order indeed ensures that all transitions are only forwards in the sorted states. So I think it's fine to fix this test bug and keep the simpler iterative solution here?

3. Should we continue to limit the size of the Automaton?

I don't think we should.

2. Perhaps we should check if the automaton contains cycles, and throw an IllegalArgumentException if it does?

That would be nice, though it really is illegal usage of the API -- can you detect that w/o much added cost in the iterative solution?

[tang-hi]

Thank you, @mikemccand, for your valuable feedback. I've made updates to my pull request based on your suggestions.

Regarding the modification of the test code, it appears there's no need for adjustments. The current test is correct. It was my initial implementation that had issues, which caused the test failure.

As for checking if the automaton contains cycles, I have incorporated this check in the updated version. I have used a BitSet to track whether the current state is already on the stack. If we encounter a state that's already on the stack, we've detected a cycle. You can refer to this resource for more information on cycle detection.

While adding the BitSet introduces a small overhead and slightly increases the complexity of the code, I believe the code remains understandable. Please let me know if you have any further suggestions or feedback. 😄

[rmuir]

i really like it, thanks for doing this!

code is still simple, but more robust (heap instead of stack!) and the cycle detection is a really nice bonus. thanks for the unit test for that.

i made a cosmetic suggestion.

Also, should we add a basic test that the toposort works correctly to TestOperations? TestAutomaton does exercise it too, but I'm not sure how well or how easy to debug if it fails :)

[dweiss]

Use ArrayDeque instead of java.util.Stack (synchronized methods)?

[uschindler]

Use ArrayDeque instead of java.util.Stack (synchronized methods)?

We should but this on forbiddenapis.

[tang-hi]

@rmuir I have added a unit test to evaluate the toposort functionality. I developed a method that generates a random automaton, and you can include a cycle by passing the hasCycle argument. This will allow us to test both toposort and cycle detection.

@uschindler @dweiss Thank you for your advice. I have made the change from using Stack to Deque

[uschindler]

Only 2 minor comments, but it's approved in general.

[tang-hi]

Maybe we could merge this pull request if there are no other comments.

[uschindler]

I can take care of that, but I would like to get a final "go" by @rmuir. :-)
Uwe

[romseygeek]

minor nit: this should say 'returns the number of states in the final sorted list' I think?

[tang-hi]

Yes, My bad.Already fix that

[rmuir]

looks great. thanks again.

[tang-hi]

Thank you everyone for your valuable comments on my PR.

[uschindler]

Thanks @tang-hi for the nice work!

[uschindler]

One additional thing as my last comment: We moved from recursive to iterative, but we still have a stack (deque). It is not so limited like the OS stack by the Java VM, but still for some FSA it could consume a lot of heap space.

Any thoughts?

[tang-hi]

One additional thing as my last comment: We moved from recursive to iterative, but we still have a stack (deque). It is not so limited like the OS stack by the Java VM, but still for some FSA it could consume a lot of heap space.

Any thoughts?

The only approach I can think of to limit heap consumption is to restrict the size of deque. However, I believe this will only occur in special cases. In addition, if users wish to perform topological sorting on large automata and are prepared to accommodate the associated memory consumption and time costs, I don't think we should impose any limitations.If there are any better suggestions, I am open to them at any time.

[mikemccand]

I think the RAM consumption is OK, but, we should clearly advertise it in the javadocs for this method? Since we detect cycles we will never have an "attempt to use infinite RAM".

[uschindler]

I think we have the same problem with other methods in this class that were transformed to be iterative earlier.
So we should maybe make a comment about what types of automatons could consume lot of space. But for end users it might be important to have this in our query classes.

[tang-hi]

I think adding a comment would be great. I have already submitted a new pull request. @uschindler @mikemccand

[uschindler]

Looks fine, I can merge that later, no need for additional changes.txt.

[uschindler]

Hi,
I had to fix the test to use TestUtil#nextInt(min, max) as Java 11 has no two-parameter Random#nextInt(origin, bound). I had to substract 1 from bound as its is inclusive in TestUtil.

[rmuir]

@tang-hi @uschindler @mikemccand I think now that there are no more recursive algorithms in src/java we can now move Operations.MAX_RECURSION_LEVEL to AutomatonTestUtil in src/test? This test helper still uses it only because it has some simple recursive implementations used for validating correctness. And I think it is a good step to be able to move it to just tests!

[mikemccand]

@tang-hi @uschindler @mikemccand I think now that there are no more recursive algorithms in src/java we can now move Operations.MAX_RECURSION_LEVEL to AutomatonTestUtil in src/test? This test helper still uses it only because it has some simple recursive implementations used for validating correctness. And I think it is a good step to be able to move it to just tests!

Yay, what a great milestone!

[tang-hi]

@rmuir Already raise a PR to move that.