Skip to content

Fix the problem that the dependency tree is different when DF and BF …#172

Closed
weibo1995 wants to merge 5 commits intoapache:masterfrom
weibo1995:bf-bugfix
Closed

Fix the problem that the dependency tree is different when DF and BF …#172
weibo1995 wants to merge 5 commits intoapache:masterfrom
weibo1995:bf-bugfix

Conversation

@weibo1995
Copy link
Contributor

@weibo1995 weibo1995 commented May 5, 2022

Fix the problem that the dependency tree is different when DF and BF strategies are adopted when a dependency package has no children dependency.
When BF is adopted, when a dependent package has no children dependency, it will not continue to call the doRecurse() method, that is, it will not call the CacheManager.cachewinner() to put this dependency package into the winnerGAs object, which affects the analysis results of dependency packages of the same GA. The current fix is to call the CacheManager.cachewinner() method when a dependent package has no children dependency and it is not skip resolution, so that the dependent package can be found in winnerGAs.

We take the dependency in the following figure as an example to illustrate the generation process of the problem.
image

At org eclipse. aether. internal. impl. Bfdependencycollector #collectdependencies() mainly has two stages: processdependency() and transformer.transformGraph( node, context )。
In the first stage of process dependency:

  1. When parsing to D1, descriptorResult.getDependencies().isEmpty(), so doRecurse() will not be executed. Therefore, args.skipper.cache() will not be executed. The D1 will not exist in winnerGAs in the end.
    image

  2. When parsing D2 of the same layer, because there is no same GA in winnerGAs, D2 and its children dependencies ( G1 and H1) will be parsed.

  3. When resolving to G2 on the same layer as G1, because winnerGAs already has G1, the resolution of the children dependencies of G2 will be skipped, that is, H2 will not be resolved.
    Then, to the second stage, transformgraph:

  4. Because D1 has the same depth as D2, D1 wins, that is, D2 and its children are eliminated, including G1

  5. Because G1 was eliminated, G2 won.

Finally, the final dependency tree obtained by BF strategy is as follows:
image

But the dependency tree obtained by DF is as follows:
image

That is, in the final generated dependency tree, BF has less children dependency of G2 than DF.

thanks.

Signed-off-by: hongbo.weihb hongbo.weihb@alibaba-inc.com

…strategies are adopted when a dependency package has no indirect dependency.

Signed-off-by: hongbo.weihb <hongbo.weihb@alibaba-inc.com>
@cstamas cstamas requested review from cstamas and michael-o May 5, 2022 08:29
@cstamas
Copy link
Member

cstamas commented May 5, 2022

@weibo1995 could you craft a project that produces different trees on current master? As we could add it as UT...

@weibo1995
Copy link
Contributor Author

@weibo1995 could you craft a project that produces different trees on current master? As we could add it as UT...

OK, let me add UT, thanks.

@michael-o
Copy link
Member

@weibo1995 Very interesting, thank you!

…ion dependencies is empty

Signed-off-by: hongbo.weihb <hongbo.weihb@alibaba-inc.com>
@weibo1995
Copy link
Contributor Author

weibo1995 commented May 5, 2022

@weibo1995 could you craft a project that produces different trees on current master? As we could add it as UT...

586eaad
3599112

The dependency tree used in the unit test is the same as the following figure:
image

The final expected result is the same as the figure below:
image

thanks.

…or dependencies is empty

Signed-off-by: hongbo.weihb <hongbo.weihb@alibaba-inc.com>
@cstamas
Copy link
Member

cstamas commented May 5, 2022

Note to myself: after this PR is merged, we should pull up the "common" tests from DfDependencyCollectorTest and BfDependencyCollectorTest as they should cover pretty much same stuff, leave in UT classes only the specific tests. For example, this new test should be pulled up, as both should produce "same result"!

weibo1995 added 2 commits May 5, 2022 22:04
Signed-off-by: hongbo.weihb <hongbo.weihb@alibaba-inc.com>
Signed-off-by: hongbo.weihb <hongbo.weihb@alibaba-inc.com>
@cstamas
Copy link
Member

cstamas commented May 5, 2022

Locally verified: took test code and test resources from this PR (but NOT the FIX), and copied it to BF and DF UT. I expected BF to fail and DF to pass.

And that exactly happened:

[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   BfDependencyCollectorUseSkipTest>BfDependencyCollectorTest.testDescriptorDependenciesEmpty:601->BfDependencyCollectorTest.assertEqualSubtree:133->BfDependencyCollectorTest.assertEqualSubtree:163->BfDependencyCollectorTest.assertEqualSubtree:163->BfDependencyCollectorTest.assertEqualSubtree:163->BfDependencyCollectorTest.assertEqualSubtree:155 path: [gid:a:jar:ver (), gid:e:jar:ver (compile), gid:f:jar:ver (compile), gid:g:jar:2 (compile)], expected: [gid:h:jar:2 (compile)], actual: [] expected:<1> but was:<0>
[INFO] 
[ERROR] Tests run: 299, Failures: 1, Errors: 0, Skipped: 0

@michael-o
Copy link
Member

JIRA issue please

@caiwei-ebay2
Copy link

@weibo1995

Thanks for fixing!
Are you evaluating the performance difference between BF and DF? Did you find any improvement when running projects in your company?

@weibo1995
Copy link
Contributor Author

@weibo1995

Thanks for fixing! Are you evaluating the performance difference between BF and DF? Did you find any improvement when running projects in your company?

For applications with compilation time greater than 10 minutes, the effect is very good.
thanks.

@caiwei-ebay2
Copy link

@weibo1995

Really appreciate your feedback, could you please let me know how many performance improvement you've witnessed for your project? Maybe before, after.

I'm actually the author of this patch (I'm unable to login with original caiwei-ebay account with 2FA, so using this caiwei-ebay2 account :) ), with projects in our company, there is 30% to 70% performance gain if projects take minutes to build.

@weibo1995
Copy link
Contributor Author

weibo1995 commented May 8, 2022

@weibo1995

Really appreciate your feedback, could you please let me know how many performance improvement you've witnessed for your project? Maybe before, after.

I'm actually the author of this patch (I'm unable to login with original caiwei-ebay account with 2FA, so using this caiwei-ebay2 account :) ), with projects in our company, there is 30% to 70% performance gain if projects take minutes to build.


At present, 5 or 6 long tail applications have been piloted, and the compilation time can be reduced by half, that is, about 100% performance gain. Later, we will use BF for more applications, and then feedback and communicate.
thanks.

@caiwei-ebay2
Copy link

@weibo1995

Really appreciate your feedback! This solution does help for enterprise projects and was inspired from one project in our company that would fail with OOM (20G memory +) when running mvn clean install, this means this solution also reduced memory cost.

@jira-importer
Copy link

Resolve #1355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants