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

feat: enable parallel branch building #647

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

galuszkak
Copy link
Contributor

Hi,

One of the things that became an issue at some points is that building multiple branches with bigger model takes more and more time (our builds are getting to 40 mins). To solve this problem we would like to propose new flag --parallel in generate-site command that would use native git executable to establish git worktree for every branch that will be used and build those branches in parallel from separate branch git worktrees. jGit as of today doesn't have a support for worktrees - hence need to use native git executable.

This should effectively make builds faster in most scenarios.

Any feedback mostly appreciated.

@jenspav
Copy link
Collaborator

jenspav commented Dec 2, 2024

Out of interest, how many branches do you usually use and how much time do you gain with this PR applied?
Looking briefly at the PR itself, if I'm honest, I would prefer to not introduce a second way to interact with a git repository.

@galuszkak
Copy link
Contributor Author

galuszkak commented Dec 2, 2024

@jenspav I will check the build numbers - will try run them tomorrow. We have around 25+ branches. Completely understand that using other way to interact with repo is not a great way - I would personally also prefer to use just jGit - sadly they don't have support worktrees - and I don't like idea of cloning 25 times a repo, that I already cloned once and have all branches.

@qtzar
Copy link
Contributor

qtzar commented Dec 2, 2024

I have a similar issue with a large number of branches. I resolved it by adding some code to my build pipeline to only build branches that changes within the last X days so I can reduce the number of branches.

A parallel builder might be handy and would allow more branches to be built.

    - name: Find Old Branches
        uses: phpdocker-io/github-actions-delete-abandoned-branches@v2
        id: old_branches
        with:
          github_token: ${{ github.token }}
          last_commit_age_days: 30
          # IMPORTANT : dry_run is set to 'yes' so that nothing is actually deleted.
          # DO NOT CHANGE THIS SETTING
          dry_run: yes

    - name: Fix Array
        id: fix_array
        uses: actions/github-script@v7
        with:
          script: |
            const branchArray = ${{steps.old_branches.outputs.deleted_branches}}
            return branchArray.toString();

    - name: Build Site
        run: docker run --user root -t --rm -e PLANTUML_LIMIT_SIZE=16384 -v ${{ github.workspace }}:/var/model ghcr.io/avisi-cloud/structurizr-site-generatr generate-site  -g https://github.com/XXXXXXX/workspace.git -u ${{ github.actor }} -p ${{ secrets.GITHUB_TOKEN }} -w workspace.dsl --all-branches --exclude-branches ${{steps.fix_array.outputs.result}} -d master -a assets
        shell: bash

@galuszkak
Copy link
Contributor Author

We use feature branches for having architecture decisions that are pending and not implemented yet, so we need to build them always - we merge them to main branch only after they are implemented. Hence we can't rely on time based branches - as sometimes implementation takes longer and HLD/ADD/ADR should be available for teams and architects.

@galuszkak
Copy link
Contributor Author

galuszkak commented Dec 2, 2024

After trying to test this branch with our own repo, I think I uncovered bug in https://github.com/structurizr/java - but would be grateful if someone can confirm my suspicion - which effectively makes me unable to provide the numbers that you asked for @jenspav.

We are extensively using !scripts in our workspace views - for tags filtering. While doing so there is something with jRuby or Structurizr itself that screws with context, when doing this in parallel - in sequential mode works perfectly.

To reproduce that when using my branch just do:

$ ./gradlew run --args="generate-site -par -g https://github.com/FlyrInc/structurizr-site-generatr.git -b test1,test2,test3,test4,test5 -d test1 -w docs/example/workspace.dsl"

test1-5 branches are exactly the same. They contain simple script ( link: https://github.com/FlyrInc/structurizr-site-generatr/blob/test1/docs/example/views.rb ) that is exactly the same on couple of views:

tags = defined?(tags) ? tags : ""
def tags_filter(tags=[])
  puts "Tags: #{tags}"
end
tags_filter(tags=tags.split(","))

When running this without -par argument this works perfectly fine. But when it's done in parallel I've got exception:

javax.script.ScriptException: Error during evaluation of Ruby in /Users/kamil/Projects/Archive/structurizr-site-generatr/build/model-clone/2/docs/example/views.rb at line 9: (NoMethodError) undefined method `split' for ["tag1", "tag2", "tag3"]:Array
        at org.jruby.embed.jsr223.JRubyEngine.wrapRaiseException(JRubyEngine.java:277)
        at org.jruby.embed.jsr223.JRubyEngine.doEval(JRubyEngine.java:102)
        at org.jruby.embed.jsr223.JRubyEngine.eval(JRubyEngine.java:126)
        at com.structurizr.dsl.ScriptDslContext.run(ScriptDslContext.java:70)
        at com.structurizr.dsl.ExternalScriptDslContext.end(ExternalScriptDslContext.java:23)
        at com.structurizr.dsl.StructurizrDslParser.endContext(StructurizrDslParser.java:1167)
        at com.structurizr.dsl.StructurizrDslParser.parse(StructurizrDslParser.java:242)
        at com.structurizr.dsl.StructurizrDslParser.parse(StructurizrDslParser.java:140)
        at nl.avisi.structurizr.site.generatr.CreateStructurizrWorkspaceKt.createStructurizrWorkspace(CreateStructurizrWorkspace.kt:10)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository$lambda$4(GenerateSiteCommand.kt:130)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository$lambda$5(GenerateSiteCommand.kt:117)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.ForEachOps$ForEachTask.compute(ForEachOps.java:291)
        at java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
        at java.base/java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:667)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:160)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:174)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
        at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:765)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository(GenerateSiteCommand.kt:117)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.execute(GenerateSiteCommand.kt:77)
        at kotlinx.cli.ArgParser.parse(ArgParser.kt:657)
        at kotlinx.cli.ArgParser.parse(ArgParser.kt:530)
        at nl.avisi.structurizr.site.generatr.AppKt.main(App.kt:13)
Caused by: org.jruby.exceptions.NoMethodError: (NoMethodError) undefined method `split' for ["tag1", "tag2", "tag3"]:Array
        at RUBY.<main>(/Users/kamil/Projects/Archive/structurizr-site-generatr/build/model-clone/2/docs/example/views.rb:9)
Exception in thread "main" com.structurizr.dsl.StructurizrDslParserException: Error running script at views.rb, caused by javax.script.ScriptException: Error during evaluation of Ruby in /Users/kamil/Projects/Archive/structurizr-site-generatr/build/model-clone/2/docs/example/views.rb at line 9: (NoMethodError) undefined method `split' for ["tag1", "tag2", "tag3"]:Array at line 199 of /Users/kamil/Projects/Archive/structurizr-site-generatr/build/model-clone/2/docs/example/workspace.dsl: }
        at com.structurizr.dsl.StructurizrDslParser.parse(StructurizrDslParser.java:1065)
        at com.structurizr.dsl.StructurizrDslParser.parse(StructurizrDslParser.java:140)
        at nl.avisi.structurizr.site.generatr.CreateStructurizrWorkspaceKt.createStructurizrWorkspace(CreateStructurizrWorkspace.kt:10)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository$lambda$4(GenerateSiteCommand.kt:130)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository$lambda$5(GenerateSiteCommand.kt:117)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:184)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.ForEachOps$ForEachTask.compute(ForEachOps.java:291)
        at java.base/java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754)
        at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387)
        at java.base/java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:667)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateParallel(ForEachOps.java:160)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateParallel(ForEachOps.java:174)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)
        at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
        at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:765)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.generateSiteForModelInGitRepository(GenerateSiteCommand.kt:117)
        at nl.avisi.structurizr.site.generatr.GenerateSiteCommand.execute(GenerateSiteCommand.kt:77)
        at kotlinx.cli.ArgParser.parse(ArgParser.kt:657)
        at kotlinx.cli.ArgParser.parse(ArgParser.kt:530)
        at nl.avisi.structurizr.site.generatr.AppKt.main(App.kt:13)

Which suggest something happening in structurizr-dsl that is responsible for that behaviour. Are they just reusing scripting context - and because of this, somehow tags token is becoming Array? Should I raise this in structurizr-dsl first?

@galuszkak
Copy link
Contributor Author

After getting a fix for jRuby I could finally run some numbers on our usual workspace with 25 branches that you asked for @jenspav.

On Apple Macbook Pro M1:

  • Build without patch 14m 5s
  • Build parallel 10m 15s.

I assume on CI/CD we would have gains around 10-15 minutes based on this patch.

@jenspav
Copy link
Collaborator

jenspav commented Dec 19, 2024

Thanks for the numbers. To be honest, I prefer to not have the complexity and that this PR introduces upstream. I agree though that the improvements are noticeable, but it's not major.

That said, I have a a different angle to look at performance. Currently we recreate the diagram cache for each branch, may be we can reuse the cache properly across branches. We had this at one point, but it got reversed because of issues. See #363 and #349 . Reverting that PR and solving the related issue properly should gain you much more time than building in parallel.
Also related #352

@galuszkak
Copy link
Contributor Author

@jenspav
Copy link
Collaborator

jenspav commented Dec 19, 2024

Thanks. To be sure you could revert #349 and run it against your setup. That way we would get an idea if the time for diagram generation is indeed not the issue.

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.

3 participants