core/instrumentation: optimisations

No need to spawn $(date) to get the number of seconds-since-EPOCH, as bash's printf can do it as easily.

This is just a micro-optimisation, though. Probably not noticeable.

Reported-by Trent Piepho <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Trent Piepho <[email protected]>

---
 package/pkg-generic.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

package/pkg-generic.mk +0 -0

index 9eddaeee57..2a82025a04 100644

package/pkg-generic.mk +0 -1

               >>"$(BUILD_DIR)/build-time.log"
 endef
 GLOBAL_INSTRUMENTATION_HOOKS += step_time
--

2.14.1

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Quoted 17 lines

No need to spawn $(date) to get the number of seconds-since-EPOCH, as bash's printf can do it as easily.

This is just a micro-optimisation, though. Probably not noticeable.

Reported-by Trent Piepho <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Trent Piepho <[email protected]>

---
 package/pkg-generic.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

package/pkg-generic.mk +0 -0

index 9eddaeee57..2a82025a04 100644

package/pkg-generic.mk endef · line 53

Quoted 7 lines

package/pkg-generic.mk +2 -2

@@ -49,8 +49,8 @@ endef
 
 # Time steps
 define step_time
-       printf "%s:%-5.5s:%-20.20s: %s\n"           \
-              "$$(date +%s)" "$(1)" "$(2)" "$(3)"  \
+       printf "%(%s)T:%-5.5s:%-20.20s: %s\n"           \
+              -1 "$(1)" "$(2)" "$(3)"  \

Should you note somewhere that this needs bash 4.2? I remember some re-working of bash patches in the past to avoid the use of associative arrays as they were not supported in the oldest version of bash supported by buildroot. Is bash 4.2 ok now?

See http://tiswww.case.edu/php/chet/bash/NEWS for when features were added to bash.

Quoted 6 lines

Quoted 5 lines

define step_time

package/pkg-generic.mk +2 -2

-       printf "%s:%-5.5s:%-20.20s: %s\n"           \
-              "$$(date +%s)" "$(1)" "$(2)" "$(3)"  \
+       printf "%(%s)T:%-5.5s:%-20.20s: %s\n"           \
+              -1 "$(1)" "$(2)" "$(3)"  \

Quoted 5 lines

Should you note somewhere that this needs bash 4.2? I remember some re-working of bash patches in the past to avoid the use of associative arrays as they were not supported in the oldest version of bash supported by buildroot. Is bash 4.2 ok now?

OK, I'm dropping this change, then. Thanks for noticing. :-)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

As part of the build, we run some instrumentation hooks to gather statistics about the usage of the target/, staging/ and host/ directories, so that we can generate reports for the user, that shows:
- for each file, what package installed it,
- for each package,the size that it installed.

In so doing, we run a double md5 pass on all files of the affected directories. These passes were mostly invisible when we were only scanning target/, but has greatly increased in time now that we also scan staging/ and host/ (but only in the corresponding _CMDS, of course).

This md5 wsa mostly aimed at catching packages that would "cheat" with mtime/atime/ctime somehow. They can't really cheat on md5, though [0].

Timings however speak for themselves, with this defconfig (slightly biggish-but-still-manageable build) [1].

host/ 20965 files 1.2GiB
staging/ 4715 files 333MiB
target/ 1801 files 44MiB

All instrumentation steps, using md5:    19min 27s
All instrumentation steps, using mtime:  14min 45s
No instrumentation step at all:          14min 31s

So, using mtime is an almost-5min improvement, i.e. about 25% faster, while removing all instrumentation steps does not gain that much more...

So, we switch to using mtime, because in the end that's still good-enough for our use-case: generating some graphs. It is not mission-critical, and if a graph is slightly off, that's not biggy. It can anyway be attributed to a broken package's buildsystem, which should get fixed.

However, we lose the ability to track directories. Non-empty directories can be tracked back by a bit of scripting, but empty directories are simply not caught. If we were to also look for directories using mtime, we would catch parents of installed files:

- /foo/bar/ exists
- a package installs /foo/bar/buz
- mtime of /foo/bar/ is changed to account for the nex file in it.

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

As part of the build, we run some instrumentation hooks to gather statistics about the usage of the target/, staging/ and host/ directories, so that we can generate reports for the user, that shows:
- for each file, what package installed it,
- for each package,the size that it installed.

Quoted 7 lines

So, we switch to using mtime, because in the end that's still good-enough for our use-case: generating some graphs. It is not mission-critical, and if a graph is slightly off, that's not biggy. It can anyway be attributed to a broken package's buildsystem, which should get fixed.
- /foo/bar/ exists
- a package installs /foo/bar/buz
- mtime of /foo/bar/ is changed to account for the nex file in it.

Playing around with this, I noticed two other issues:

- It doesn't work for packages using rsync to install, E.G. skeleton-init-common as rsync also sets the mtime to match the source files

- It breaks for <pkg>-reinstall

I don't think either of those are really big issues compared to the huge slowdown, but it is worth noticing.

package/pkg-generic.mk GLOBAL_INSTRUMENTATION_HOOKS += step_time · line 70

Quoted 6 lines

package/pkg-generic.mk +6 -0

+define step_pkg_size_inner
+	cd $(2); \
+	find . \( -type f -o -type L \) \
+		-newer $($(PKG)_DIR)/.stamp_built \
+		-exec printf '$(1),%s\n' {} + \
+		>> $(BUILD_DIR)/packages-file-list$(3).txt

What find version are you using? My fileutils find (and the busybox applet) use 'l' for symlinks, so I've changed it to that.

Committed with that fixed (and a few tweaks to the commit message), thanks.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Quoted 3 lines

- It doesn't work for packages using rsync to install, E.G. skeleton-init-common as rsync also sets the mtime to match the source files

We could maybe tell rsycn not to do that, then?

Quoted 1 line

- It breaks for <pkg>-reinstall

Well, we can't guarantee anything except with a clean build from scratch anyway.

Quoted 2 lines

I don't think either of those are really big issues compared to the huge slowdown, but it is worth noticing.

Well, the -reinstall was already not working correctly, because the list pf files before/after would be alsmost the same, and the md5-diff would miss all the laready-installed files for the package.

The rsync issue is new, but we can "fix" it in a later patch, then, for those packages like the skeletons, by using the --no-times option for example.

However, if a third-party package internally uses rsync as its install method, we're screwed. But who would be insane enough to do that? ;-]

Alternatively, we could use ctime instead of mtime, maybe? Or check both?

Quoted 9 lines

package/pkg-generic.mk GLOBAL_INSTRUMENTATION_HOOKS += step_time · line 70

Quoted 6 lines

package/pkg-generic.mk +6 -0

+define step_pkg_size_inner
+	cd $(2); \
+	find . \( -type f -o -type L \) \
+		-newer $($(PKG)_DIR)/.stamp_built \
+		-exec printf '$(1),%s\n' {} + \
+		>> $(BUILD_DIR)/packages-file-list$(3).txt

What find version are you using? My fileutils find (and the busybox applet) use 'l' for symlinks, so I've changed it to that.

Doh, the s/L/l/ is still uncomitted here. Dang...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Hi,

Quoted 5 lines

Quoted 3 lines

- It doesn't work for packages using rsync to install, E.G. skeleton-init-common as rsync also sets the mtime to match the source files

We could maybe tell rsycn not to do that, then?

Yes, possibly.

Quoted 4 lines

Quoted 1 line

- It breaks for <pkg>-reinstall

Well, we can't guarantee anything except with a clean build from scratch anyway.

True. We could potentionally do a touch on the stamp file before running find, but it is somewhat icky.

Quoted 10 lines

Quoted 2 lines

I don't think either of those are really big issues compared to the huge slowdown, but it is worth noticing.

Well, the -reinstall was already not working correctly, because the list pf files before/after would be alsmost the same, and the md5-diff would miss all the laready-installed files for the package.

The rsync issue is new, but we can "fix" it in a later patch, then, for those packages like the skeletons, by using the --no-times option for example.

Yes. I guess we cannot use --no-times unconditionally in SYSTEM_RSYNC, as the mtime shouldn't be touched for source files so OVERRIDE_SRCDIR doesn't rebuild too much.

Quoted 2 lines

However, if a third-party package internally uses rsync as its install method, we're screwed. But who would be insane enough to do that? ;-]

And even so, the breakage is not so bad.

It really depends on what you use these files for.

The original use case for the target list was rootfs size analysis. In the discussion I have seen comments like missing a few files is not that important here, but I disagree: if the missing file is 2MB large, it is a big problem.

Another use in-tree is to check for check-uniq-files. While this is a non-critical feature, it's a pity if it would not detect problems because the lists are inaccurate.

But there are out-of-tree uses too. The most obvious usage is simply to understand which package was responsible for a given file, even separate from size analysis.

But there are also derived use cases. For example we are using the target list in order to extract some packages from the root filesystem. For example, instead of on the root filesystem (initramfs or NOR flash), they should end up on the NAND flash. A script gets as input the list of packages to extract this way, and uses the list to get the right associated files.

I'm sure there are other use cases.

The current timestamp-based approach not guaranteeing an accurate list is problematic for many such uses. And as you already mentioned, since we don't have full control over the build steps done in any given package, we don't know which timestamps they will use. There may be very good reasons to install certain files with their original timestamp and not the one from the build.

Hello,

Quoted 20 lines · Thomas De Schampheleire

It really depends on what you use these files for.

The original use case for the target list was rootfs size analysis. In the discussion I have seen comments like missing a few files is not that important here, but I disagree: if the missing file is 2MB large, it is a big problem.

Another use in-tree is to check for check-uniq-files. While this is a non-critical feature, it's a pity if it would not detect problems because the lists are inaccurate.

But there are out-of-tree uses too. The most obvious usage is simply to understand which package was responsible for a given file, even separate from size analysis.

But there are also derived use cases. For example we are using the target list in order to extract some packages from the root filesystem. For example, instead of on the root filesystem (initramfs or NOR flash), they should end up on the NAND flash. A script gets as input the list of packages to extract this way, and uses the list to get the right associated files.

Quoted 7 lines

I'm sure there are other use cases.

The current timestamp-based approach not guaranteeing an accurate list is problematic for many such uses. And as you already mentioned, since we don't have full control over the build steps done in any given package, we don't know which timestamps they will use. There may be very good reasons to install certain files with their original timestamp and not the one from the build.

These are all valid concerns, but what do you suggest ?

The current approach of hashing all files clearly doesn't scale, as a significant amount of build time is now spent on hashing files.

Quoted 7 lines

Quoted 6 lines

The current timestamp-based approach not guaranteeing an accurate list is problematic for many such uses. And as you already mentioned, since we don't have full control over the build steps done in any given package, we don't know which timestamps they will use. There may be very good reasons to install certain files with their original timestamp and not the one from the build.

Quoted 5 lines

These are all valid concerns, but what do you suggest ?

The current approach of hashing all files clearly doesn't scale, as a significant amount of build time is now spent on hashing files.

I can only observe that previously, when we still only listed the target files, the impact did not seem to be that bad, and the concerns about impact on build time arose with the creation of staging and host lists.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

These are all valid concerns, but what do you suggest ?

The current approach of hashing all files clearly doesn't scale, as a significant amount of build time is now spent on hashing files.

Hi all,

Quoted 1 line · Peter Korsgaard

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

The current timestamp-based approach not guaranteeing an accurate list is problematic for many such uses. And as you already mentioned, since we don't have full control over the build steps done in any given package, we don't know which timestamps they will use. There may be very good reasons to install certain files with their original timestamp and not the one from the build.

Quoted 5 lines

The current timestamp-based approach not guaranteeing an accurate list is problematic for many such uses. And as you already mentioned, since we don't have full control over the build steps done in any given package, we don't know which timestamps they will use. There may be very good reasons to install certain files with their original timestamp and not the one from the build.

We could revert to the original before/after file list approach, and just use
something like "stat -c '%N %Y %f'" instead of md5sum.

Though I certainly like Trent's idea of installing each package in a separate
directory, too. With PPS we're more or less there anyway. We'd just need to step up the adoption rate of PPS then :-)

Timings however speak for themselves, with this defconfig (slightly biggish-but-still-manageable build) [1].

host/ 20965 files 1.2GiB
staging/ 4715 files 333MiB
target/ 1801 files 44MiB

All instrumentation steps, using md5:    19min 27s
All instrumentation steps, using mtime:  14min 45s
No instrumentation step at all:          14min 31s

So, using mtime is an almost-5min improvement, i.e. about 25% faster, while removing all instrumentation steps does not gain that much more...

Now, we also change the way we handle symlinks. Previously, we would hash the file pointed to by the symlink. Now, we only look at the mtime of the symlink itself, which still detects modifications.

After fixing s/L/l/, here are the timings for a 15 minute build:

configure 510.29
build 166.12

extract                   45.53
other                     43.88
hostinstall               35.43
check_host_rpath          33.37
check_bin_arch            28.30
targetinstall             23.37
stageinstall              15.97

step_pkg_size 9.49

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Thomas" == Thomas Petazzoni <[email protected]> writes:

Hello,

Quoted 4 lines · Trent Piepho

After fixing s/L/l/, here are the timings for a 15 minute build:

configure 510.29
build 166.12

Quoted 8 lines

extract                   45.53
other                     43.88
hostinstall               35.43
check_host_rpath          33.37
check_bin_arch            28.30
targetinstall             23.37
stageinstall              15.97

step_pkg_size 9.49

Thanks for those measurements. I still find it a bit annoying that we spend 1m10sec on instrumentation related topics on a 15 minutes build. The use of ccache makes the build time lower than it normally is, and therefore makes the instrumentation cost even more visible, but using ccache is a valid use case.

Should we move the check_host_rpath and check_bin_arch checks as finalize hooks, instead of running them at the end of each package installation ? We can always get the package that installed the "bogus" file through the package-file-list*.txt files, no ?

Quoted 12 lines

Quoted 12 lines

After fixing s/L/l/, here are the timings for a 15 minute build:

configure 510.29
build 166.12

extract                   45.53
other                     43.88
hostinstall               35.43
check_host_rpath          33.37
check_bin_arch            28.30
targetinstall             23.37
stageinstall              15.97

step_pkg_size 9.49

Quoted 5 lines

Thanks for those measurements. I still find it a bit annoying that we spend 1m10sec on instrumentation related topics on a 15 minutes build. The use of ccache makes the build time lower than it normally is, and therefore makes the instrumentation cost even more visible, but using ccache is a valid use case.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Trent" == Trent Piepho <[email protected]> writes:

After fixing s/L/l/, here are the timings for a 15 minute build:

configure 510.29
build 166.12

extract                   45.53
other                     43.88
hostinstall               35.43
check_host_rpath          33.37
check_bin_arch            28.30
targetinstall             23.37
stageinstall              15.97

step_pkg_size 9.49

Without ccache, the build step would be somewhat longer.

Quoted 3 lines

Now, we also change the way we handle symlinks. Previously, we would hash the file pointed to by the symlink. Now, we only look at the mtime of the symlink itself, which still detects modifications.

This patch also fixes a problem the previous system had w.r.t. broken symlinks. They did not show up as owned by any packages. Now they are owned by the package that creates the link. It also means a package that replaced a file which was linked to does not also appear, incorrectly, to replace all the links as well.