Re: [Buildroot] [PATCH RESEND v2] ppd-merge: speed up per-package-rsync

[Brandon Maier via buildroot <buildroot@buildroot.org>]

Hi Yann,

On 2024.06.13 07:28, yann.morin@orange.com wrote:
> Brandon, All,
> 
> On 2024-06-12 18:22 +0000, Brandon Maier via buildroot spake thusly:
> > From: Brandon Maier via buildroot <buildroot@buildroot.org>
> > The per-package-rsync stage can add a significant amount of time to
> > builds. They can also be annoying as the target-finalize and
> > host-finalize targets are the slowest and run on every `make all`, which
> > is used frequently for partial rebuilds.
> > 
> > The per-package-rsync is slow because it launches a new rsync for each
> > source tree, and each rsync must rescan the destination directory and
> > potentially overwrite files multiple times. We can instead merge all the
> > rsync calls down into one call, and rsync is smarter about scanning all
> > the source directories and only copying over the files it needs to.
> > 
> > We feed the source trees to rsync in reverse-order, as this preserves
> > the original behaviour. I.e. when using multiple rsyncs, the last source
> > tree would overwrite anything in the destination. Now when using a
> > single rsync, we put the last tree first as rsync will select the first
> > file it finds.
> > 
> > This only supports the 'copy' mode, which is used in the finalize step.
> > The 'hardlink' mode requires specifying each source tree with the
> > --link-dest flag but enforces a maximum of 20 trees.
> 
> I think that last sentence for the hardlink mode no longer applies with
> this new iteration, since the hardlink case is no longer handled.

I was trying to explain why this patch only updates `per-package-rsync
copy`, and does not update `per-package-rsync hardlink`. Because rsync's
hardlink support works differently. But we can drop this paragraph if
it's confusing.

> 
> > Below is a benchmark running the host-finalize target for a build with
> > 200 packages.
> > 
> > Benchmark 1: before copy
> >   Time (mean ± σ):     27.171 s ±  0.777 s    [User: 6.170 s, System: 14.830 s]
> >   Range (min … max):   26.343 s … 28.566 s    10 runs
> > 
> > Benchmark 2: after copy
> >   Time (mean ± σ):      6.296 s ±  0.196 s    [User: 2.874 s, System: 5.600 s]
> >   Range (min … max):    6.094 s …  6.709 s    10 runs
> 
> Impressive! I don't get quite the same improvements here, as my build is
> smaller, but I get from ~21.715 to ~9.080 (5 runs each), which is more
> than twice (~2.4x) faster. Good! :-)
> 
> To be noted: your numbers as well as mines imply a running host-finalize
> after a previous completed, successful build, of course.
> 
> So, I also did a test to see what happens when the host/ directory did
> not previously exist:
>     $ make; rm -rf output/host/
>     $ time make host-finalize
> 
> And even in that case, I get about a 2.2x improvement too, which is also
> interesting.

Yes, a lot of the slowdown has to do with running multiple rsyncs. Each
rsync scans over output/host/ and overwrites any files from its
per-package directory that doesn't match. So even when output/host/
already exists, a lot of files get overwritten anyway.

This will get worse once your host-finalize patch is applied. As it will
rewrite many files in output/host/ so they no longer match the
per-package/ files.

> 
> Finally, I compared the content of host/ in both cases:
> 
>     $ make; mv output/host{,.slow}
>     $ make; mv output/host{,.fast}
>     $ diff durN output/host{.slow,.fast}
> 
> The only results are dangling symlinks that are only valid at runtime
> (e.g. [XXX]/staging/dev/stdout that points to ../proc/self/stdout).

I tested this and it appears `diff` is printing errors because it can't
find the symlink. But the symlinks themselves are identical, so this
shouldn't be a regression.

  $ diff -drN -- output/host{.slow,.fast}
  diff: output/host.slow/arm-buildroot-linux-gnueabihf/sysroot/dev/fd: No such file or directory
  diff: output/host.fast/arm-buildroot-linux-gnueabihf/sysroot/dev/fd: No such file or directory

  $ ls -al output/host{.slow,.fast}/arm-buildroot-linux-gnueabihf/sysroot/dev/fd
  lrwxrwxrwx 1 blmaier genusers 15 Jun 11 21:47 output/host.fast/arm-buildroot-linux-gnueabihf/sysroot/dev/fd -> ../proc/self/fd
  lrwxrwxrwx 1 blmaier genusers 15 Jun 11 21:47 output/host.slow/arm-buildroot-linux-gnueabihf/sysroot/dev/fd -> ../proc/self/fd

> 
> Tested-by: Yann E. MORIN <yann.morin@orange.com>
> Reviewed-by: Yann E. MORIN <yann.morin@orange.com>
> 
> Regards,
> Yann E. MORIN.

Thank you for testing!
Brandon Maier

> 
> > Summary
> >   after copy ran
> >     4.32 ± 0.18 times faster than before copy
> > 
> > Cc: Herve Codina <herve.codina@bootlin.com>
> > Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Reviewed-by: Herve Codina <herve.codina@bootlin.com>
> > Signed-off-by: Brandon Maier <brandon.maier@collins.com>
> > ---
> > v1: https://patchwork.ozlabs.org/project/buildroot/patch/20231127224139.35969-1-brandon.maier@collins.com/
> > 
> > v2:
> > - Simplify logic for 'copy' mode
> > - Drop support for 'hardlink' mode
> > ---
> >  package/pkg-utils.mk | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> > index d1964299af..3ab9e9fe52 100644
> > --- a/package/pkg-utils.mk
> > +++ b/package/pkg-utils.mk
> > @@ -221,18 +221,12 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> >  # $4: literal "copy" or "hardlink" to copy or hardlink files from src to dest
> >  define per-package-rsync
> >  	mkdir -p $(3)
> > -	$(foreach pkg,$(1),\
> > -		rsync -a \
> > -			--hard-links \
> > -			$(if $(filter hardlink,$(4)), \
> > -				--link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(2)/, \
> > -				$(if $(filter copy,$(4)), \
> > -					$(empty), \
> > -					$(error per-package-rsync can only "copy" or "hardlink", not "$(4)") \
> > -				) \
> > -			) \
> > -			$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
> > -			$(3)$(sep))
> > +	$(if $(filter hardlink,$(4)), \
> > +		$(foreach pkg,$(1),\
> > +			rsync -a --hard-links --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ \
> > +				$(PER_PACKAGE_DIR)/$(pkg)/$(2)/ $(3)$(sep)), \
> > +		printf "%s/$(2)/\n" $(1) | tac \
> > +			| rsync -a --hard-links --files-from=- --no-R -r $(PER_PACKAGE_DIR) $(3))
> >  endef
> >  
> >  # prepares the per-package HOST_DIR and TARGET_DIR of the current
> > 
> > ---
> > base-commit: 12b2860d7562d5cf1b7492b58f4f10f0405444c4
> > change-id: 20240612-ppd-merge-b4-8c623ba7ab90
> > 
> > Best regards,
> > -- 
> > Brandon Maier <brandon.maier@collins.com>
> > 
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
> 
> -- 
>                                         ____________
> .-----------------.--------------------:       _    :------------------.
> |  Yann E. MORIN  | Real-Time Embedded |    __/ )   | /"\ ASCII RIBBON |
> |                 | Software  Designer |  _/ - /'   | \ / CAMPAIGN     |
> | +33 638.411.245 '--------------------: (_    `--, |  X  AGAINST      |
> | yann.morin (at) orange.com           |_="    ,--' | / \ HTML MAIL    |
> '--------------------------------------:______/_____:------------------'
> 
> ____________________________________________________________________________________________________________
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
> 
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot