testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.98k stars 1.02k forks source link

Optimize Graph.removeFromNodes() #1391

Closed numeralnathan closed 6 years ago

numeralnathan commented 7 years ago

TestNG Version

6.11

Actual behavior

After fixing issue #1390, Graph.removeFromNodes() consumes 60% of the time for TestRunner.initMethods(). Here's the current code.

  private void removeFromNodes(List<Node<T>> nodes, Node<T> node) {
    nodes.remove(node);
    for (Node<T> n : nodes) {
      n.removePredecessor(node.getObject());
    }
  }

Expected behavior

Graph.removeFromNodes() could be optimized if each Node kept a list of its successors. Then only the successors would have to be processed.

As I pointed out in #1390, the predecessor map is almost always empty and hence the successors would almost always be empty. Thus, the impact from Graph.removeFromNodes() could be almost entirely eliminated.

Is the issue reproductible on runner?

Test case sample

This isn't possible unless I give you all 30,000 test cases. Perhaps, using a DataProvider you could create a test suite with 30,000 test cases and then track how long it takes to execute TestRunner.initMethods().

juherr commented 7 years ago

@nathanila You've made the harder part of the job. Why not proposing a pull request?

numeralnathan commented 7 years ago

I don't know how to do a pull request. Is there some documentation you can point me to on how to do this in Eclipse?

numeralnathan commented 7 years ago

Here's the diff of the changes necessary. These changes eliminates Graph.removeFromNodes() from the profile. The start up time is a lot faster!

Diff.pdf

krmahadevan commented 7 years ago

@nathanila - Sometime back I created a blog that talks about the general set of things that you need to do to setup yourself for contributing into open source projects. You can read read here.

For TestNG, after you have added the code, you need to add some tests, and include those tests into https://github.com/cbeust/testng/blob/master/src/test/resources/testng.xml You then would include the reference to the bug you fixed in this file.

To run the build, from the command prompt you just need to invoke ./build-with-gradle. If you are on windows, then you can perhaps just invoke gradlew --daemon --stacktrace clean build test

I am not sure if it makes sense for someone else to take your diff and submit it as a pull request because

steveprentice commented 6 years ago

Likely dupe of #1710.

juherr commented 6 years ago

We expect the issue will be resolved by #1718.