XcodeGen icon indicating copy to clipboard operation
XcodeGen copied to clipboard

Support for parallel target generation

Open asifmohd opened this issue 4 years ago • 8 comments

Background

  • In my current work project, we have around 55 targets being generated using Xcodegen. Out of which 24 targets are targets with no dependency on any other target.
  • After running the time profiler using my project.yml and a debug xcodegen binary, I found out that most of the time was being spent in the Glob.exploreDirectories section.
  • The 55 targets which we have migrated are just approximately 1/3rd of all the targets our app supports today, so the time taken to run Xcodegen would only increase proportionally to the number of targets we have in the future.
  • So the motivation was to be able to generate atleast these 24 targets in parallel to get a good speed boost because Glob can be executed in parallel for such targets.

Overview of changes to code in this PR

  • I started from bottom up, so the attempt was to make getSourceMatches in SourceGenerator.swift, be able to run asynchronously in multiple threads for each target
    • This required making all the functions in the calling chain accept completion handlers, instead of directly returning the generated result.
  • The changes heavily rely on GCD to dispatch to global queues, with QOS as userInitiated, along with heavy use of DispatchGroups, to block queue execution when successive code depend on a result from an async completion handler being executed.
  • The main queue does not perform any operations while the targets are being generated but rather is blocked waiting for the targets to be generated before proceeding to the Writing project phase. Instead of blocking the main queue, we could probably show progress to the user on how many targets have been generated in a separate PR.
    • Had to block the main queue because:
      • Main queue is serial and we want to generate multiple targets, so we need to use a global async queue.
      • I felt we should keep the main queue clear of any background/processing operations, so everything related to generating the targets, is done in a background thread.
      • The execute function in GenerateCommand.swift is a synchronous function, if we return early, the xcodegen command exits as well, so we have to make the main queue wait so that the command thinks, that processing is still going on.
  • Introduced the Atomic class inspired from an objc.io article
    • These are required to make some of the shared variables such as rootGroups in SourceGenerator.swift and more, be thread safe, ie reading and writing happens using a serial queue.
      • A future improvement/refactor would be try and make more and more of these variables independently stored in a struct for each target, have to explore the codebase more to understand if that is possible.
  • Moved the getAllDependenciesPlusTransitiveNeedingEmbedding from SourceGenerator.swift to Target.swift, so that the code in SourceGenerator.swift reduces a bit, and to be able to reuse this function to figure out the order of generating targets, by reusing and calling this function on each target itself, ex: target.getAllDependencies(usingProject:)
    • This change required moving shouldEmbedDependencies as well and marking it as a public function
    • Let me know if this change can be given in a separate PR to reduce the diff on this PR
  • Because of the introduction of dispatch groups, and dispatch queues, there is a lot of diff, with just indentation changes, so apologies for the large diff in advance 🙏

Overview of changes related to tests in this PR

  • To minimise the amount of code changes in the test targets, the helper functions such as generateXcodeProject in PBXProjGeneratorTests.swift have been modified to convert the completion handler based functions to be serial functions, which return a single value instead of accepting and calling the completion handlers
  • All the existing tests pass, but no new tests have been added yet. Will add new tests, after an initial round of review.

Results

  • For the 55 targets, on my MBP 2018 version, it used to take 14 seconds for the xcodegen command to finish.
  • Now after these changes it takes approx 7 seconds, a 50% improvement in project generation times
  • These improvements will vary depending on how many targets you have, and how many of them could be generated in parallel.

asifmohd avatar May 17 '20 15:05 asifmohd

We have a project.yml file that has 276 targets, where 275 depend on a single other target. This change resulted in no speed up or slow down for us 😕.

brentleyjones avatar May 18 '20 13:05 brentleyjones

We have a project.yml file that has 276 targets, where 275 depend on a single other target. This change resulted in no speed up or slow down for us 😕.

I should be able to do something about this, basically I'll have to modify the parallelization logic to sort by the number of dependencies a target has, and then pick the one with the least dependencies and so on, so that more and more targets can be parallelized in each cycle. Right now I just pick the last target from the array, which is probably why you are not seeing an improvement. Will try to make this change this coming weekend 🙂

asifmohd avatar May 18 '20 16:05 asifmohd

@keith, would you mind testing this on the Lyft codebase?

yonaskolb avatar May 30 '20 01:05 yonaskolb

We have a project.yml file that has 276 targets, where 275 depend on a single other target. This change resulted in no speed up or slow down for us

@brentleyjones with my most recent commit, ideally the dependency resolution logic should pick up the target with no dependencies, then targets with just 1 dependencies, and so on. Ideally this should help in your project, assuming that single target which all other targets depend on, does not have any dependencies. Therefore the single target will be built first, and then in the next cycle, we'll build all those targets which depend on this single target, and if there are no other dependencies in the targets in this cycle, all 275 of them should build in parallel 🤞

asifmohd avatar May 30 '20 17:05 asifmohd

There is a lot of scope in improving the dependency resolution of targets, like we could probably build functionality, which would parse all the targets, and find the most commonly depended target, build that and it's dependencies first, so that rest of the targets can be parallelized. Kinda like our own build system for generating targets.

asifmohd avatar May 30 '20 17:05 asifmohd

@asifmohd It's now about 10% slower than master for us. And when we make it so we instead have 275 independent targets we get into a deadlock and it never returns.

brentleyjones avatar Jun 01 '20 13:06 brentleyjones

I tested this and also hit the deadlock, here's the main thread in that case:

Call graph:
    2498 Thread_11217851   DispatchQueue_1: com.apple.main-thread  (serial)
    + 2498 start  (in libdyld.dylib) + 1  [0x7fff6cdda7fd]
    +   2498 main  (in xcodegen) + 194  [0x1019e5752]
    +     2498 XcodeGenCLI.execute(arguments:)  (in xcodegen) + 37  [0x1019eb9b5]
    +       2498 CLI.go()  (in xcodegen) + 119  [0x1019a4b47]
    +         2498 CLI.go(with:)  (in xcodegen) + 1352  [0x1019a5118]
    +           2498 protocol witness for Command.execute() in conformance ProjectCommand  (in xcodegen) + 9  [0x1019ead99]
    +             2498 ProjectCommand.execute()  (in xcodegen) + 1261  [0x1019eac8d]
    +               2498 GenerateCommand.execute(specLoader:projectSpecPath:project:)  (in xcodegen) + 6268  [0x1019e8d5c]
    +                 2498 _dispatch_group_wait_slow  (in libdispatch.dylib) + 43  [0x7fff6cd81f61]
    +                   2498 _dlock_wait  (in libdispatch.dylib) + 44  [0x7fff6cd81cd6]
    +                     2498 __ulock_wait  (in libsystem_kernel.dylib) + 10  [0x7fff6cf1c9be]

keith avatar Jun 02 '20 16:06 keith

Thanks for testing this out everyone 🙏 I will try to add some more tests related to parallel target generation, will try to include the scenarios where it's failing for you guys. I have not been able to find time these past few weeks, and there are still a few other work related commitments i have to take care of. So progress might be slow, will ping back here, once the PR is ready for review again.

asifmohd avatar Jun 25 '20 07:06 asifmohd