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

Avoid build.ninja changes due to order of hash table iteration #7900

Merged
merged 3 commits into from
Nov 18, 2020

Conversation

bonzini
Copy link
Contributor

@bonzini bonzini commented Oct 26, 2020

The ordering of elements in sets and of keys in dictionaries cannot be relied upon, because the hash values are randomized by Python. Whenever the processing of meson.build or the operation of the backend is affected by this ordering, random changes in the command line can occur and cause ninja to rebuild a lot of files unnecessarily.

This pull request fixes the instances that affect QEMU, and which were causing about 40% of the build to be repeated after touch ../meson.build (most of them due to sourcesets).

@jpakkane
Copy link
Member

This should probably use OrderedMap and OrderedSet consistently rather than fixing it later with sorted.

@bonzini
Copy link
Contributor Author

bonzini commented Oct 26, 2020

Sometimes it should (and the patches do), especially if that affects the build command line directly. In fact for sourcesets the pull request is already using OrderedSet. However, I'm not sure that should always be the case.

For dictionaries in meson.build I am afraid of users relying on the iteration order being the same as the assignment order, which I don't think should be guaranteed; it feels wrong that Meson dictionaries are OrderedDicts rather than just associative arrays. In this case, sorting is just the easy answer to the immediate need I had, which is to stabilize the order of iteration. Yes, perhaps dictionaries shouldn't be a SortedDict either but it seemed to me that it's less likely for meson.build to rely on sorting order than on assignment order.

In the remaining cases (depfiles in the first patch and ninjabackend) Meson uses set just to get rid of duplicates but it doesn't need the original order. In these cases I chose to sort just because it produces neater results. It is easier to scan for a missing file if you are debugging meson.build, for example; using an OrderedSet to keep the order didn't strike me as particularly useful. In the case of ninjabackend, in addition, the order of the resulting set would also depend on implementation details of the backend which should not necessarily be visible in build.ninja.

@bonzini
Copy link
Contributor Author

bonzini commented Oct 28, 2020

By the way, there is a precedent for sorting the keys in the output in mesonlib.dump_conf_header. The third patch in this PR merely extends the behavior to (some parts of) build.ninja.

@bonzini bonzini force-pushed the stabilize-hash branch 2 times, most recently from 36633ff to 3e39971 Compare November 9, 2020 08:01
The order of elements in sets cannot be relied upon, because the hash
values are randomized by Python.  Whenever sets are converted to lists
we need to keep their order stable, or random changes in the command line
cause ninja to rebuild a lot of files unnecessarily.  To stabilize them,
use either sort or OrderedSet.  Sorting is not always applicable, but it
can be faster because it's done in C and it can produce slightly nicer
output.
The order of keys in dictionaries cannot be relied upon, because the hash
values are randomized by Python.  Whenever we iterate on dictionaries and
meson.build generates a list during the iteration, the different iteration
orders may cause random changes in the command line and cause ninja to
rebuild a lot of files unnecessarily.
…cies

These do not go into the command line, and therefore do not matter
for the purpose of avoiding unnecessary rebuilds after meson is
rerun.  However, they complicate the task of finding differences
between build lines across meson reruns.

So take the easy way out and sort everything after | and ||.
With this change, there is absolutely no change in QEMU's 40000-line
build.ninja file after meson is rerun.
@jpakkane
Copy link
Member

I thought about this some more and it is in fact important that we preserve the order given. For example:

executable(, dependencies: [lib1_dep, lib2_dep])

If both lib1 and lib2 are static libraries and 1 depends on symbols from 2, it must come first on the link line at least with GNU ld. Now it could be said that this is wrong as 1 should have a dependency on 2 but sometimes that does not work (especially if those are internal helper libraries).

Now the fact that we have not gotten bug reports about this implies that this is not actually happening in the real world. Still, I'd prefer OrderedSet as doing that fixes this iteration order everywhere, even for code that might get added in the future.

@bonzini
Copy link
Contributor Author

bonzini commented Nov 18, 2020

You're probably thinking of commit "ninjabackend: stabilize order of dependencies and order-only dependencies" which changes the order of ninja dependencies, not Meson dependency objects. That is, in a rule

rule output.o: c_COMPILER output.c | header1.h header2.h || order-only1.in order-only2.in

The header1.h header2.h part and the order-only1.in order-only2.in parts are kept sorted in order to stabilize the output. The order of these does not matter.

The sourceset patch is the only one that deals with Meson dependencies and also the only one that uses OrderedSet, so we should be good.

@jpakkane jpakkane merged commit 1c582a9 into mesonbuild:master Nov 18, 2020
@bonzini bonzini deleted the stabilize-hash branch January 15, 2021 17:41
bonzini added a commit to bonzini/qemu that referenced this pull request Mar 14, 2021
The main advantage of 0.57 is that it fixes
mesonbuild/meson#7900, thus avoiding unnecessary
rebuilds after running meson.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request Mar 15, 2021
The main advantage of 0.57 is that it fixes
mesonbuild/meson#7900, thus avoiding unnecessary
rebuilds after running meson.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request May 13, 2021
The main advantage of 0.57 is that it fixes
mesonbuild/meson#7900, thus avoiding unnecessary
rebuilds after running meson.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request May 20, 2021
The main advantage of 0.57 is that it fixes
mesonbuild/meson#7900, thus avoiding unnecessary
rebuilds after running meson.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
yangzhon pushed a commit to intel/qemu-sgx that referenced this pull request Sep 8, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request Sep 20, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request Oct 5, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

- 0.59: Utility methods for feature objects

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
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.

None yet

2 participants