Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GoMod: Fix the algorithm for handling cycles #5628

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Aug 2, 2022

The previous algorithm exhibits the following issues when run on graphs
with cycles, in particular when the amount of edges is large:

  1. Cycles of length 1 lead to infinite recursion.
  2. The algorithm is inefficient in terms of execution time and memory
    allocation, as it creates a copy of the
    predecessorNodes set per recursion.
  3. When run against [1] the execution of toPackageReferenceForest()
    didn't finish within 15 minutes, causing high CPU load and memory
    consumption.

If GoMod used the dependency graph format already, the solution would be
as simple as calling DependencyGraphBuilder.breakCycles(). Fix the
problem by copying the code of DependencyGraphBuilder.breakCycles() as
a temporary quick fix. This saves effort, because refactoring GoMod to
use the dependency graph is planned anyway [2], which will allow for
removing that copied code again.

[1] https://github.com/ossf/scorecard
[2] #4249

Fixes #5627.

@fviernau fviernau requested a review from a team as a code owner August 2, 2022 14:18
@fviernau fviernau requested a review from tsteenbe August 2, 2022 14:18
@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #5628 (3f9e8a6) into main (918e64e) will increase coverage by 0.06%.
The diff coverage is 54.05%.

❗ Current head 3f9e8a6 differs from pull request most recent head 1467603. Consider uploading reports for the commit 1467603 to get more accurate results

@@             Coverage Diff              @@
##               main    #5628      +/-   ##
============================================
+ Coverage     67.83%   67.90%   +0.06%     
+ Complexity     2177     2164      -13     
============================================
  Files           271      271              
  Lines         15854    15863       +9     
  Branches       2809     2823      +14     
============================================
+ Hits          10754    10771      +17     
+ Misses         3967     3957      -10     
- Partials       1133     1135       +2     
Flag Coverage Δ
funTest-analyzer-docker 73.87% <76.92%> (+0.01%) ⬆️
funTest-non-analyzer 46.09% <ø> (ø)
test 32.10% <0.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
analyzer/src/main/kotlin/managers/GoMod.kt 61.90% <54.05%> (+4.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fviernau fviernau force-pushed the gomod-properly-break-cycles branch 2 times, most recently from effa5f9 to ec1d003 Compare August 2, 2022 14:49
@dgutson
Copy link

dgutson commented Aug 2, 2022

Thanks @fviernau!, is there an issue already created to "refactor GoMod to
use the dependency graph"?

@qequ
Copy link

qequ commented Aug 2, 2022

Tested the analyze tool with the Scorecard repo and works fine without the need of adding extra memory 👌

oheger-bosch
oheger-bosch previously approved these changes Aug 3, 2022
analyzer/src/main/kotlin/managers/GoMod.kt Outdated Show resolved Hide resolved
@tsteenbe
Copy link
Member

tsteenbe commented Aug 3, 2022

Thanks @fviernau!, is there an issue already created to "refactor GoMod to use the dependency graph"?

@dgutson Yes, there is see #4249

oheger-bosch
oheger-bosch previously approved these changes Aug 3, 2022
The previous algorithm exhibits the following issues when run on graphs
with cycles, in particular when the amount of edges is large:

1. Cycles of length 1 lead to infinite recursion.
2. The algorithm is inefficient in terms of execution time and memory
   allocation, as it creates a copy of the
   `predecessorNodes` set per recursion.
3. When run against [1] the execution of `toPackageReferenceForest()`
   didn't finish within 15 minutes, causing high CPU load and memory
   consumption.

If GoMod used the dependency graph format already, the solution would be
as simple as calling `DependencyGraphBuilder.breakCycles()`. Fix the
problem by copying the code of `DependencyGraphBuilder.breakCycles()` as
a temporary quick fix. This saves effort, because refactoring GoMod to
use the dependency graph is planned anyway [2], which will allow for
removing that copied code again.

[1] https://github.com/ossf/scorecard
[2] #4249

Fixes #5627.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the gomod-properly-break-cycles branch from 69de79a to 1467603 Compare August 3, 2022 07:06
@fviernau fviernau enabled auto-merge (rebase) August 3, 2022 07:07
@fviernau fviernau merged commit aec19fc into main Aug 3, 2022
@fviernau fviernau deleted the gomod-properly-break-cycles branch August 3, 2022 07:34
@mnonnenmacher mnonnenmacher added the release notes Changes that should be mentioned in release notes label Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Changes that should be mentioned in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GoMod: Runs out of memory when analyzing the 'ossf/scorecard' project hosted on GitHub
6 participants