Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494883002:
GN: Makes GN output deterministic (Closed)
|
Created:
5 years ago by Zachary Forman Modified:
5 years ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN: Makes GN output deterministic
Sorts various output just prior to printing to ensure that GN output
is deterministic.
The overall effect on performance is relatively small, with
runtime over 50 runs varying as follows on my Z840:
+-------+-------+-------+
| | old | new |
+-------+-------+-------+
| mean | 1415 | 1430 |
+-------+-------+-------+
| stdev | 58.0 | 70.5 |
+-------+-------+-------+
To verify results:
$ gn gen out/Default; mv out out-1
$ gn gen out/Default; mv out out-2
$ diff -qr out-1 out-2
The diff should be empty.
Initial discussion can be seen at https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI.
BUG=565075
Committed: https://crrev.com/98ec25a8fb3f710d43d60b1bcdc5a1475aab136f
Cr-Commit-Position: refs/heads/master@{#364954}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Responses to comments #Patch Set 3 : Adds newline #Patch Set 4 : Reverts removal of sort #Patch Set 5 : Moves sort into ninja_toolchain_writer.cc for consistency #
Total comments: 6
Patch Set 6 : Simplifies to just sorting #Patch Set 7 : Tweaked some comments for clarity #
Total comments: 14
Patch Set 8 : Shrinking scope #Patch Set 9 : Shrinking scope #
Total comments: 5
Patch Set 10 : Code delousing #
Messages
Total messages: 34 (9 generated)
zforman@google.com changed reviewers: + mithro@mithis.com
Hi Tim! Please review this CL. Hopefully no major disasters :) --Zac
Description was changed from ========== GN: Makes GN output deterministic Adds a PointerSet class that sorts elements contained within it by the dereferenced pointers, in order to have deterministic ordering instead of pointer based ordering, and changes all instances of std::set<Target*> to PointerSet<Target>. Additionally adds sorts to several places in order to make them deterministic by ordering dependencies / files in a consistent way. The overall effect on performance is - surprisingly - positive, with runtime over 50 runs varying as follows on my Z840: +-------+-------+-------+ | | old | new | +-------+-------+-------+ | mean | 1604 | 1462 | +-------+-------+-------+ | stdev | 40.2 | 110.4 | +-------+-------+-------+ To verify results: $ gn gen out/Default; mv out out-1 $ gn gen out/Default; mv out out-2 $ diff -qr out-1 out-2 The diff should be empty. BUG=565075 ========== to ========== GN: Makes GN output deterministic Adds a PointerSet class that sorts elements contained within it by the dereferenced pointers, in order to have deterministic ordering instead of pointer based ordering, and changes all instances of std::set<Target*> to PointerSet<Target>. Additionally adds sorts to several places in order to make them deterministic by ordering dependencies / files in a consistent way. The overall effect on performance is - surprisingly - positive, with runtime over 50 runs varying as follows on my Z840: +-------+-------+-------+ | | old | new | +-------+-------+-------+ | mean | 1604 | 1462 | +-------+-------+-------+ | stdev | 40.2 | 110.4 | +-------+-------+-------+ To verify results: $ gn gen out/Default; mv out out-1 $ gn gen out/Default; mv out out-2 $ diff -qr out-1 out-2 The diff should be empty. Initial discussion can be seen at https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI. BUG=565075 ==========
Hi Zack, Mostly looks okay, but we'll need the GN developers feedback. I added a bunch of initial comments. It would be good to figure out *why* this change makes things faster (and get someone else to reproduce it). Tim 'mithro' Ansell https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_build_writer.cc File tools/gn/ninja_build_writer.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_build_writer... tools/gn/ninja_build_writer.cc:198: // Keep the files sorted to ensure output is deterministic. If you are assuming the input is sorted here, it probably makes sense to DCHECK it here? https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc File tools/gn/ninja_writer.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:32: if (!writer.WriteToolchains(&all_settings, &default_targets, err)) Doesn't sorting in NinjaWriter::WriteToolchains mean you don't need to sort here? https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:35: // Sort the targets so that outputs have a consistent order. Can we get a test for this? https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:37: [](const Target* a, const Target* b) { return *a < *b; }); Can you use your comparator here instead of a lambda? https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:87: std::sort(i->second.begin(), i->second.end(), Same as above... https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h File tools/gn/pointer_set.h (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h#newc... tools/gn/pointer_set.h:15: class DereferenceComparator { Does something like this already exist in stl at all? It feels like it might? https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h#newc... tools/gn/pointer_set.h:35: template <typename T, typename Comparator = std::less<T>> Do we actually need a full wrapper class here? Could we get away with a templated typedef or macro? Do we need this to be templated? The only type we seem to use is a `const Target*` object. Should we have a TargetPtrSet type? https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... File tools/gn/pointer_set_unittest.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... tools/gn/pointer_set_unittest.cc:3: // found in the LICENSE file. You could probably use a bunch more tests here. Do we need checks around null pointers at all? https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... tools/gn/pointer_set_unittest.cc:17: EXPECT_EQ(1, **ps.begin()); These EXPECT_EQs are kind of hard to read. There are "Container Matchers" avaliable in gmock (which extends gtest). See https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m... I'm unsure if they'll make this cleaner. https://codereview.chromium.org/1494883002/diff/1/tools/gn/target.cc File tools/gn/target.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/target.cc#newcode557 tools/gn/target.cc:557: bool Target::operator<(const Target& other) const { I'm unsure if we want an implicit less than operator. Check with the gn developers if they are okay with this.
I've uploaded a new snapshot addressing your comments. Slightly concerned about testing for ninja_writer.cc - I don't see how to test it without dragging a heap of other classes into it, and even then it seems fairly hairy. Hopefully I'm missing something, but I suspect that's why it - unlike other ninja*writer.cc - has no existing _unittest.cc. https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_build_writer.cc File tools/gn/ninja_build_writer.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_build_writer... tools/gn/ninja_build_writer.cc:198: // Keep the files sorted to ensure output is deterministic. On 2015/12/03 at 05:20:31, mithro wrote: > If you are assuming the input is sorted here, it probably makes sense to DCHECK it here? Poorly phrased - we place the filepaths into a set to ensure that they're sorted. I've updated the comment. https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc File tools/gn/ninja_writer.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:32: if (!writer.WriteToolchains(&all_settings, &default_targets, err)) On 2015/12/03 at 05:20:31, mithro wrote: > Doesn't sorting in NinjaWriter::WriteToolchains mean you don't need to sort here? Other way around - more than just WriteToolchains calls this function. Good catch, though. https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:35: // Sort the targets so that outputs have a consistent order. On 2015/12/03 at 05:20:31, mithro wrote: > Can we get a test for this? I don't think so. It percolates through a stack of other methods, each of which instantiates more NinjaBuildWriters. It is probably possible, but I don't know how. https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:37: [](const Target* a, const Target* b) { return *a < *b; }); On 2015/12/03 at 05:20:31, mithro wrote: > Can you use your comparator here instead of a lambda? Yeah, might as well. It'll be a little nicer. https://codereview.chromium.org/1494883002/diff/1/tools/gn/ninja_writer.cc#ne... tools/gn/ninja_writer.cc:87: std::sort(i->second.begin(), i->second.end(), On 2015/12/03 at 05:20:31, mithro wrote: > Same as above... Acknowledged https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h File tools/gn/pointer_set.h (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h#newc... tools/gn/pointer_set.h:15: class DereferenceComparator { On 2015/12/03 at 05:20:31, mithro wrote: > Does something like this already exist in stl at all? It feels like it might? Nope. Which is a shame as it's sometimes useful. https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set.h#newc... tools/gn/pointer_set.h:35: template <typename T, typename Comparator = std::less<T>> On 2015/12/03 at 05:20:31, mithro wrote: > Do we actually need a full wrapper class here? Could we get away with a templated typedef or macro? > > Do we need this to be templated? The only type we seem to use is a `const Target*` object. Should we have a TargetPtrSet type? Yep - I've factored it down to a much smaller thing. I think templating makes sense given my rewrite, if only because std::set is also templated. I'd not typedef to TargetPtrSet; callers either used std::set or typedef'd to something else themselves. https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... File tools/gn/pointer_set_unittest.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... tools/gn/pointer_set_unittest.cc:3: // found in the LICENSE file. On 2015/12/03 at 05:20:31, mithro wrote: > You could probably use a bunch more tests here. > > Do we need checks around null pointers at all? I've refactored so that we should be as safe as a set is. Checking for null pointers maybe a good idea - I've added a DCHECK that neither parameter is a nullptr so that the error message is more sensible than a segfault. https://codereview.chromium.org/1494883002/diff/1/tools/gn/pointer_set_unitte... tools/gn/pointer_set_unittest.cc:17: EXPECT_EQ(1, **ps.begin()); On 2015/12/03 at 05:20:31, mithro wrote: > These EXPECT_EQs are kind of hard to read. > > There are "Container Matchers" avaliable in gmock (which extends gtest). See https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m... > > I'm unsure if they'll make this cleaner. Acknowledged. Entire set of tests is dead now anyway. https://codereview.chromium.org/1494883002/diff/1/tools/gn/target.cc File tools/gn/target.cc (right): https://codereview.chromium.org/1494883002/diff/1/tools/gn/target.cc#newcode557 tools/gn/target.cc:557: bool Target::operator<(const Target& other) const { On 2015/12/03 at 05:20:31, mithro wrote: > I'm unsure if we want an implicit less than operator. Check with the gn developers if they are okay with this. Acknowledged. I'll leave this for the GN developers to comment on - my thought is that a target (or perhaps, rather, an item) has a natural ordering based on the label.
Looks like you missed something.....
-----------------------------------------------------
tansell@tansell-z620-l2:/fast/chrome/src$ for i in {1..5}; do time
./out-fast/Release/gn gen out/Default >> gn-after; m
v out out-$i; done
tansell@tansell-z620-l2:/fast/chrome/src$ diff -qr out-1 out-2
Files out-1/Default/toolchain.ninja and out-2/Default/toolchain.ninja differ
tansell@tansell-z620-l2:/fast/chrome/src$ diff -qr out-2 out-3
Files out-2/Default/toolchain.ninja and out-3/Default/toolchain.ninja differ
tansell@tansell-z620-l2:/fast/chrome/src$ diff -ru out-1 out-2
diff -ru out-1/Default/toolchain.ninja out-2/Default/toolchain.ninja
--- out-1/Default/toolchain.ninja 2015-12-03 21:35:10.288186900 +1100
+++ out-2/Default/toolchain.ninja 2015-12-03 21:35:11.388203334 +1100
@@ -131,8 +131,8 @@
subninja obj/components/open_from_clipboard/open_from_clipboard.ninja
subninja obj/sql/sql.ninja
subninja obj/ui/events/platform/platform.ninja
-subninja obj/components/password_manager/content/common/common.ninja
subninja
obj/third_party/WebKit/Source/bindings/scripts/cached_lex_yacc_tables.ninja
+subninja obj/components/password_manager/content/common/common.ninja
subninja obj/dbus/dbus_test_server.ninja
subninja obj/blimp/engine/common/common.ninja
subninja obj/extensions/components/native_app_window/native_app_window.ninja
@@ -158,8 +158,8 @@
subninja obj/content/shell/copy_shell_resources.ninja
subninja obj/mojo/runner/host/host.ninja
subninja
obj/third_party/WebKit/Source/bindings/modules/interfaces_info_individual_modules.ninja
-subninja obj/components/omnibox/browser/browser.ninja
subninja
obj/third_party/WebKit/Source/bindings/modules/modules_global_constructors_idls.ninja
+subninja obj/components/omnibox/browser/browser.ninja
subninja obj/third_party/leveldatabase/env_chromium_unittests.ninja
subninja obj/third_party/leveldatabase/leveldb_filename_test.ninja
subninja obj/components/dom_distiller/content/mojo_bindings_cpp_sources.ninja
@@ -489,8 +489,8 @@
subninja obj/third_party/WebKit/Source/modules/make_modules_generated.ninja
subninja obj/ui/app_list/app_list_demo.ninja
subninja
obj/mojo/services/tracing/public/interfaces/interfaces_cpp_sources.ninja
-subninja obj/ui/mojo/init/init.ninja
subninja
obj/third_party/WebKit/Source/bindings/core/interfaces_info_individual_core.ninja
+subninja obj/ui/mojo/init/init.ninja
subninja obj/mojo/runner/register_local_aliases_fwd.ninja
subninja obj/native_client/src/shared/serialization/serialization.ninja
subninja obj/components/mus/surfaces/surfaces.ninja
@@ -996,8 +996,8 @@
subninja
obj/third_party/WebKit/Source/bindings/core/v8/bindings_core_v8_generated.ninja
subninja obj/third_party/codesighs/maptsvdifftool.ninja
subninja obj/mojo/services/network/network_service_library.ninja
-subninja obj/cc/blink/blink.ninja
subninja
obj/third_party/WebKit/Source/bindings/modules/modules_global_objects.ninja
+subninja obj/cc/blink/blink.ninja
subninja obj/chrome/repack_locales_pack_nb.ninja
subninja obj/components/network_time/network_time.ninja
subninja obj/third_party/pdfium/third_party/pdfium_base.ninja
@@ -1595,8 +1595,8 @@
subninja obj/media/test/pipeline_integration_tests.ninja
subninja obj/mojo/public/platform/native/mgl_thunks.ninja
subninja obj/cc/proto/proto_internal_gen.ninja
-subninja obj/third_party/libvpx_new/libvpx_intrinsics_ssse3.ninja
subninja obj/third_party/WebKit/Source/bindings/modules/interfaces_info.ninja
+subninja obj/third_party/libvpx_new/libvpx_intrinsics_ssse3.ninja
subninja obj/device/vibration/mojo_bindings__generator.ninja
subninja obj/components/translate/content/browser/browser.ninja
subninja obj/gpu/command_buffer/client/gles2_c_lib_nocheck.ninja
@@ -1632,8 +1632,8 @@
subninja obj/components/devtools_http_handler/unit_tests.ninja
subninja obj/mojo/services/network/apptests.ninja
subninja obj/components/devtools_service/public/cpp/cpp.ninja
-subninja obj/third_party/webrtc/modules/audio_coding/g711.ninja
subninja
obj/third_party/WebKit/Source/bindings/modules/bindings_modules_generated_event_modules_names.ninja
+subninja obj/third_party/webrtc/modules/audio_coding/g711.ninja
subninja obj/components/handoff/handoff.ninja
subninja obj/mash/wm/public/interfaces/interfaces__generator.ninja
subninja obj/components/font_service/font_service_library.ninja
-----------------------------------------------------
It also seems a bit weird that your z840 is slower then my z620? It does look
like your patch makes gn speed less variable!?
- Before ----------------------------------------------------
Done. Wrote 3545 targets from 898 files in 1197ms
Done. Wrote 3545 targets from 898 files in 980ms
Done. Wrote 3545 targets from 898 files in 971ms
Done. Wrote 3545 targets from 898 files in 1042ms
Done. Wrote 3545 targets from 898 files in 1264ms
- After -----------------------------------------------------
Done. Wrote 3545 targets from 898 files in 1056ms
Done. Wrote 3545 targets from 898 files in 1055ms
Done. Wrote 3545 targets from 898 files in 1054ms
Done. Wrote 3545 targets from 898 files in 1050ms
Done. Wrote 3545 targets from 898 files in 1023ms
Is it possible to write a golden test like gn-format for the ninja build writing?
zforman@google.com changed reviewers: + dpranke@chromium.org
Hi, Could you please review this CL? Mithro's reviewed it to some extent, but it would be good to have some eyes more familiar with GN to have a look at it. Cheers, --Zac
dpranke@chromium.org changed reviewers: + brettw@chromium.org
I'll take a look, too, but Brett should review this.
On my machine I can notice a fairly consistent regression of 995ms (old) and 1073ms (new). I did 10 runs each, but almost all of the old ones were under 1000ms, and all of the new ones were above 1000ms. This makes the code slightly more difficult to follow, with various custom comparison operators and typedefed containers. And on the other side, I don't understand the problem we're trying to solve. Why do we need this directory to be the same every time?
Here's a question: is the only source of non-determinism in the GN output the ordering of stuff on the stamp lines? My impression is that's what you're fixing. Is there anything else? If there is, I want to understand it since that may be a bug. One approach would be to just filter out lines that look like "foo stamp:*" when computing the diff. With this you guys can do whatever you want without coordinating anything. IF that doesn't work, an approach I may like better than this patch is to sort the strings before writing the stamp line. That way the extra complexity is limited to one place, and I suspect will also be faster than what you're doing here.
On 2015/12/03 at 18:48:37, brettw wrote:
> Here's a question: is the only source of non-determinism in the GN output the
ordering of stuff on the stamp lines? My impression is that's what you're
fixing.
>
> Is there anything else? If there is, I want to understand it since that may be
a bug.
>
> One approach would be to just filter out lines that look like "foo stamp:*"
when computing the diff. With this you guys can do whatever you want without
coordinating anything.
>
> IF that doesn't work, an approach I may like better than this patch is to sort
the strings before writing the stamp line. That way the extra complexity is
limited to one place, and I suspect will also be faster than what you're doing
here.
There are four spots where non-determinism creeps in:
1) When printing stamps. This is the most obvious one, as it affects almost all
files. It's of the same type I brought up in the original discussion thread on
gn-dev.
2) When writing to the root level toolchain.ninja file. This is seen in
transposed entries, e.g. in [1] below.
3) When writing to the root level build.ninja file. This is also seen in swapped
entries, e.g. in [2] below.
4) When writing to the build.ninja.d file. This is seen in files swapped into
different locations. Additionally, the size of the file varies. I haven't shown
a diff for this because it's all in one line and huge.
I took a look at sorting - mean time 1307ms and stdev ~150. It compares
favorably with both the standard and the SortedSet approaches, while being
simpler, so it's perhaps overall better.
Out of interest, what machine are you building on? The fact we're all seeing
quite different performance characteristics is perhaps unusual.
[1] Differences in toolchain.ninja
$ diff out-{1,2}/Default/toolchain.ninja
1645d1644
< subninja obj/components/ssl_errors/ssl_errors.ninja
1647a1647
> subninja obj/components/ssl_errors/ssl_errors.ninja
1665d1664
< subninja obj/components/browser_sync/browser/browser.ninja
1667a1667
> subninja obj/components/browser_sync/browser/browser.ninja
1814d1813
< subninja obj/third_party/speech-dispatcher/speech-dispatcher.ninja
1815a1815
> subninja obj/third_party/speech-dispatcher/speech-dispatcher.ninja
2140d2139
< subninja obj/extensions/extensions_browsertests_run.ninja
2141a2141
> subninja obj/extensions/extensions_browsertests_run.ninja
2430d2429
< subninja
obj/extensions/components/javascript_dialog_extensions_client/javascript_dialog_extensions_client.ninja
2431a2431
> subninja
obj/extensions/components/javascript_dialog_extensions_client/javascript_dialog_extensions_client.ninja
[2] Differences in build.ninja
$ diff out-1/Default/build.ninja out-2/Default/build.ninja
2844,2846d2843
< build components/ssl_errors$:ssl_errors: phony
obj/components/ssl_errors/ssl_errors.stamp
< build components/ssl_errors: phony obj/components/ssl_errors/ssl_errors.stamp
< build ssl_errors: phony obj/components/ssl_errors/ssl_errors.stamp
2849a2847,2849
> build components/ssl_errors$:ssl_errors: phony
obj/components/ssl_errors/ssl_errors.stamp
> build components/ssl_errors: phony obj/components/ssl_errors/ssl_errors.stamp
> build ssl_errors: phony obj/components/ssl_errors/ssl_errors.stamp
2883,2884d2882
< build components/browser_sync/browser$:browser: phony
obj/components/browser_sync/browser/browser.stamp
< build components/browser_sync/browser: phony
obj/components/browser_sync/browser/browser.stamp
2888a2887,2888
> build components/browser_sync/browser$:browser: phony
obj/components/browser_sync/browser/browser.stamp
> build components/browser_sync/browser: phony
obj/components/browser_sync/browser/browser.stamp
3147,3149d3146
< build third_party/speech-dispatcher$:speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
< build third_party/speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
< build speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
3152a3150,3152
> build third_party/speech-dispatcher$:speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
> build third_party/speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
> build speech-dispatcher: phony
obj/third_party/speech-dispatcher/speech-dispatcher.stamp
3719a3720
> build
chrome/browser/ui/webui/engagement$:mojo_bindings__check_deps_are_all_mojom:
phony
obj/chrome/browser/ui/webui/engagement/mojo_bindings__check_deps_are_all_mojom.stamp
3722d3722
< build
chrome/browser/ui/webui/engagement$:mojo_bindings__check_deps_are_all_mojom:
phony
obj/chrome/browser/ui/webui/engagement/mojo_bindings__check_deps_are_all_mojom.stamp
4239a4240
> build third_party/libphonenumber$:proto_gen: phony
obj/third_party/libphonenumber/proto_gen.stamp
4243d4243
< build third_party/libphonenumber$:proto_gen: phony
obj/third_party/libphonenumber/proto_gen.stamp
6116d6115
< obj/components/ssl_errors/ssl_errors.stamp $
6118a6118
> obj/components/ssl_errors/ssl_errors.stamp $
6136d6135
< obj/components/browser_sync/browser/browser.stamp $
6138a6138
> obj/components/browser_sync/browser/browser.stamp $
6285d6284
< obj/third_party/speech-dispatcher/speech-dispatcher.stamp $
6286a6286
> obj/third_party/speech-dispatcher/speech-dispatcher.stamp $
6611d6610
< obj/extensions/extensions_browsertests_run.stamp $
6612a6612
> obj/extensions/extensions_browsertests_run.stamp $
6901d6900
<
obj/extensions/components/javascript_dialog_extensions_client/libjavascript_dialog_extensions_client.a
$
6902a6902
>
obj/extensions/components/javascript_dialog_extensions_client/libjavascript_dialog_extensions_client.a
$
I'm testing it on a Linux Z620 I'm curious, is the metabuild system responsible for any build non-determinism? All the things you mention here seem OK from a build reproducibility standpoint. And I also thought we had a bot that checks whether two subsequent builds are the same. It's sort of intellectually nice if the output from GN was always the same, but I wonder if we should be spending time on this if there are no actual problems.
dpranke@chromium.org changed reviewers: + maruel@chromium.org
+maruel We do have (fyi?) bots that look for deterministic builds. As I noted initially, having non-deterministic build files does not necessarily imply that the builds themselves are non-deterministic, and the latter thing is the thing that's important. But, I think having predictable, stable build file generation is intrinsically at least a little useful and is worth some code complexity. It would allow us to write some forms of integration tests for GN more easily, for example. But, yeah, it's not worth a lot of code complexity or speed hits. And, I am not at all surprised that we'd see perf differences between different machines, or between platforms (all the numbers so far are linux-only, right?). That's kinda the nature of the beast. And, I think
On 2015/12/04 at 00:08:51, dpranke wrote: > +maruel > > We do have (fyi?) bots that look for deterministic builds. > > As I noted initially, having non-deterministic build files does not necessarily imply that the builds themselves are non-deterministic, and the latter thing is the thing that's important. > > But, I think having predictable, stable build file generation is intrinsically at least a little useful and is worth some code complexity. It would allow us to write some forms of integration tests for GN more easily, for example. But, yeah, it's not worth a lot of code complexity or speed hits. > > And, I am not at all surprised that we'd see perf differences between different machines, or between platforms (all the numbers so far are linux-only, right?). That's kinda the nature of the beast. > > And, I think Yeah, our thoughts are along the lines of dpranke's. It shouldn't affect the actual deterministic-ness of builds, but is inherently nice to have. Currently as far as I know while we do have buildbots to make sure builds are deterministic, they're not enforced / have an issue that's leading to them fail. I haven't checked since last week, though. I'm less surprised by the fact there's number differences, more that we're finding opposite results on things like variability of generation time / reversed performance differences when averaged over several runs, which makes it hard to say if it's a regression or not. I guess it's just something I hadn't run into before :).
On 2015/12/04 00:25:56, Zachary Forman wrote: > On 2015/12/04 at 00:08:51, dpranke wrote: > > +maruel > > > > We do have (fyi?) bots that look for deterministic builds. > > > > As I noted initially, having non-deterministic build files does not > necessarily imply that the builds themselves are non-deterministic, and the > latter thing is the thing that's important. > > > > But, I think having predictable, stable build file generation is intrinsically > at least a little useful and is worth some code complexity. It would allow us to > write some forms of integration tests for GN more easily, for example. But, > yeah, it's not worth a lot of code complexity or speed hits. > > > > And, I am not at all surprised that we'd see perf differences between > different machines, or between platforms (all the numbers so far are linux-only, > right?). That's kinda the nature of the beast. > > > > And, I think > > Yeah, our thoughts are along the lines of dpranke's. It shouldn't affect the > actual deterministic-ness of builds, but is inherently nice to have. > > Currently as far as I know while we do have buildbots to make sure builds are > deterministic, they're not enforced / have an issue that's leading to them fail. > I haven't checked since last week, though. > > I'm less surprised by the fact there's number differences, more that we're > finding opposite results on things like variability of generation time / > reversed performance differences when averaged over several runs, which makes it > hard to say if it's a regression or not. I guess it's just something I hadn't > run into before :). Hi all I never looked at it because it was not a requirement on getting the build deterministic but it's still nice to have. For example if we want to ever have ninja to be content addressed, having GN generate deterministic output would likely be useful. I agree it's not a requirement to have this but Zachary offers to fix it and I think it can be done relatively easily. About performance, I wonder if the std::set usage worked properly (?). My C++ is quite rusty so I could have got it wrong so please confirm I'm wrong. https://codereview.chromium.org/1494883002/diff/80001/tools/gn/dereference_co... File tools/gn/dereference_comparator.h (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/dereference_co... tools/gn/dereference_comparator.h:34: using PointerSet = std::set<T*, DereferenceComparator<T, Comparator>>; AFAIK, that's what std::set<> already does (?) Otherwise this would mean the set never worked in GN, which I would be surprised that it is the case. --- CUT HERE --- #include <iostream> #include <set> struct A { A(int v) : x(v) {} int x; bool operator<(const A& that) const { return x < that.x; } }; int main() { std::set<A*> set_of_numbers; set_of_numbers.insert(new A(1)); set_of_numbers.insert(new A(2)); set_of_numbers.insert(new A(1)); for (const auto& itr : set_of_numbers) { std::cout << itr->x << std::endl; } return 0; } --- CUT HERE --- $ .../src/third_party/llvm-build/Release+Asserts/bin/clang++ a.cc -o a.out -std=c++11 && ./a.out 1 2 1 /me is puzzled. https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_build_wr... File tools/gn/ninja_build_writer.cc (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_build_wr... tools/gn/ninja_build_writer.cc:198: // Put the files into a set so that the output is sorted and hence Why not just std::sort(input_files.begin(), input_files.end()); That would be faster than creating a second structure in memory. https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_toolchai... File tools/gn/ninja_toolchain_writer.cc (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_toolchai... tools/gn/ninja_toolchain_writer.cc:65: // Sort here to ensure the files are deterministically ordered. You could DCHECK instead that the list is sorted. I'd prefer that to mutating the inputs.
On 2015/12/04 00:25:56, Zachary Forman wrote: > On 2015/12/04 at 00:08:51, dpranke wrote: > > +maruel > > > > We do have (fyi?) bots that look for deterministic builds. > > > > As I noted initially, having non-deterministic build files does not > necessarily imply that the builds themselves are non-deterministic, and the > latter thing is the thing that's important. > > > > But, I think having predictable, stable build file generation is intrinsically > at least a little useful and is worth some code complexity. It would allow us to > write some forms of integration tests for GN more easily, for example. But, > yeah, it's not worth a lot of code complexity or speed hits. > > > > And, I am not at all surprised that we'd see perf differences between > different machines, or between platforms (all the numbers so far are linux-only, > right?). That's kinda the nature of the beast. > > > > And, I think > > Yeah, our thoughts are along the lines of dpranke's. It shouldn't affect the > actual deterministic-ness of builds, but is inherently nice to have. > > Currently as far as I know while we do have buildbots to make sure builds are > deterministic, they're not enforced / have an issue that's leading to them fail. > I haven't checked since last week, though. > > I'm less surprised by the fact there's number differences, more that we're > finding opposite results on things like variability of generation time / > reversed performance differences when averaged over several runs, which makes it > hard to say if it's a regression or not. I guess it's just something I hadn't > run into before :). Hi all I never looked at it because it was not a requirement on getting the build deterministic but it's still nice to have. For example if we want to ever have ninja to be content addressed, having GN generate deterministic output would likely be useful. I agree it's not a requirement to have this but Zachary offers to fix it and I think it can be done relatively easily. About performance, I wonder if the std::set usage worked properly (?). My C++ is quite rusty so I could have got it wrong so please confirm I'm wrong.
Description was changed from ========== GN: Makes GN output deterministic Adds a PointerSet class that sorts elements contained within it by the dereferenced pointers, in order to have deterministic ordering instead of pointer based ordering, and changes all instances of std::set<Target*> to PointerSet<Target>. Additionally adds sorts to several places in order to make them deterministic by ordering dependencies / files in a consistent way. The overall effect on performance is - surprisingly - positive, with runtime over 50 runs varying as follows on my Z840: +-------+-------+-------+ | | old | new | +-------+-------+-------+ | mean | 1604 | 1462 | +-------+-------+-------+ | stdev | 40.2 | 110.4 | +-------+-------+-------+ To verify results: $ gn gen out/Default; mv out out-1 $ gn gen out/Default; mv out out-2 $ diff -qr out-1 out-2 The diff should be empty. Initial discussion can be seen at https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI. BUG=565075 ========== to ========== GN: Makes GN output deterministic Sorts various output just prior to printing to ensure that GN output is deterministic. The overall effect on performance is relatively small, with runtime over 50 runs varying as follows on my Z840: +-------+-------+-------+ | | old | new | +-------+-------+-------+ | mean | 1415 | 1430 | +-------+-------+-------+ | stdev | 58.0 | 70.5 | +-------+-------+-------+ To verify results: $ gn gen out/Default; mv out out-1 $ gn gen out/Default; mv out out-2 $ diff -qr out-1 out-2 The diff should be empty. Initial discussion can be seen at https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI. BUG=565075 ==========
I've gone through and replaced the (overly complicated) PointerSet solution with just sorting inputs immediately before their output. Hopefully this is a simpler change. The performance regression is ~1% on my z840 and ~2% on mithro's z620. Please take another look and let me know your thoughts :) https://codereview.chromium.org/1494883002/diff/80001/tools/gn/dereference_co... File tools/gn/dereference_comparator.h (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/dereference_co... tools/gn/dereference_comparator.h:34: using PointerSet = std::set<T*, DereferenceComparator<T, Comparator>>; On 2015/12/07 at 00:20:16, M-A Ruel wrote: > AFAIK, that's what std::set<> already does (?) > > Otherwise this would mean the set never worked in GN, which I would be surprised that it is the case. > > --- CUT HERE --- > #include <iostream> > #include <set> > > struct A { > A(int v) : x(v) {} > int x; > bool operator<(const A& that) const { > return x < that.x; > } > }; > > int main() { > std::set<A*> set_of_numbers; > set_of_numbers.insert(new A(1)); > set_of_numbers.insert(new A(2)); > set_of_numbers.insert(new A(1)); > for (const auto& itr : set_of_numbers) { > std::cout << itr->x << std::endl; > } > return 0; > } > --- CUT HERE --- > > $ .../src/third_party/llvm-build/Release+Asserts/bin/clang++ a.cc -o a.out -std=c++11 && ./a.out > 1 > 2 > 1 > > > /me is puzzled. So the Target*s are 'unique' in that there is a 1:1 matching between pointer and target name. It's more a situation where we have deps1 = {&target1, &target2}, deps2 = {&target3, &target2}, deps3 = {&target4, &target5, &target1}. The set prevents us from 'double including' a target, but it doesn't order our targets deterministically; it orders them by pointer order. My previous CL (now replaced with std::sorts) used the DereferenceOperator to directly compare the actual underlying targets, as opposed to just the pointers. This is more expensive (strcmp vs integer compare) but also gives deterministic ordering. https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_build_wr... File tools/gn/ninja_build_writer.cc (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_build_wr... tools/gn/ninja_build_writer.cc:198: // Put the files into a set so that the output is sorted and hence On 2015/12/07 at 00:20:16, M-A Ruel wrote: > Why not just std::sort(input_files.begin(), input_files.end()); > > That would be faster than creating a second structure in memory. Interestingly, the two vectors seem to contain a non-deterministic number of targets, and either one might contain duplicates. The purpose of the std::set here is both to sort the output and remove duplicates. This could be better commented, so I'll clarify that in the comment :). https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_toolchai... File tools/gn/ninja_toolchain_writer.cc (right): https://codereview.chromium.org/1494883002/diff/80001/tools/gn/ninja_toolchai... tools/gn/ninja_toolchain_writer.cc:65: // Sort here to ensure the files are deterministically ordered. On 2015/12/07 at 00:20:16, M-A Ruel wrote: > You could DCHECK instead that the list is sorted. I'd prefer that to mutating the inputs. This is poor phrasing on my part - they come in unsorted and leave sorted. Making a copy is probably cheap enough, though.
Thanks for the explanation, I think it's further possible to scope this CL to be smaller. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... File tools/gn/ninja_target_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:214: const std::set<const Target*>& hard_deps = target_->recursive_hard_deps(); Use the copy-constructor to save an empty initialization. std::set<const Target*> unique_deps(target_->recursive_hard_deps()); But then I read http://stackoverflow.com/a/1041939/1939810 and made me wonder a bit. STL is weird, e.g. "Convert to set (manually)" vs "Convert to set (using a constructor)". But I do hope the set(set) copy constructor is better than set(vector). I don't know what scale (especially with number of duplicates) we want to optimize for. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:221: // This just writs all toolchain deps for simplicity. If we find that writes (while at it) https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_toolcha... File tools/gn/ninja_toolchain_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_toolcha... tools/gn/ninja_toolchain_writer.cc:65: // Sort targets so that they are in a deterministic order. I'd still prefer: DCHECK( std::is_sorted(targets.begin(), targets.end(), [](const Target* a, const Target* b) { return a->label() < b->label(); })); remove the const and enforce caller to fix their code. But in fact you really don't need to change this file at all, see the other comment. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.cc File tools/gn/ninja_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:35: // Sort targets so that they are in a deterministic order. I'd prefer WriteToolchains() to return a sorted list instead. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:58: typedef std::map<Label, std::vector<const Target*> > CategorizedMap; You could use a set<> for Target* here, so they would be sorted on the fly. Then the following iterations can stay const. wdyt? https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:80: for (CategorizedMap::iterator i = categorized.begin(); i != categorized.end(); If you use a set above, you can switch to an enumeration using "const auto&"
https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... File tools/gn/ninja_target_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:214: const std::set<const Target*>& hard_deps = target_->recursive_hard_deps(); We don't have to micro-optimize, but in the GN code I try to be pretty careful since this will be called several thousand times in ~900ms. In this case, the extra_hard_deps and toolchaion_deps are almost always empty and if they aren't, they will be small. In contrast, the recursive_hard_deps can be several hundred items. I would do something like this: std::vector<const Target*> sorted_deps; for (dep : extra_hard_deps) { if (hard_deps doesn't contain dep) sorted_deps.push_back(dep) } <Same thing for toolchain deps> sorted_deps.insert(hard_deps); sort(sorted_deps) This will prevent the extra set construction just to merge with extra_hard_deps.
I've mostly made the changes you've suggested. The overall performance impact was negligible - time remains ~1430ms w/ stdev ~70ms. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... File tools/gn/ninja_target_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:214: const std::set<const Target*>& hard_deps = target_->recursive_hard_deps(); On 2015/12/07 at 18:48:54, brettw wrote: > We don't have to micro-optimize, but in the GN code I try to be pretty careful since this will be called several thousand times in ~900ms. > > In this case, the extra_hard_deps and toolchaion_deps are almost always empty and if they aren't, they will be small. In contrast, the recursive_hard_deps can be several hundred items. > > I would do something like this: > > std::vector<const Target*> sorted_deps; > for (dep : extra_hard_deps) { > if (hard_deps doesn't contain dep) > sorted_deps.push_back(dep) > } > <Same thing for toolchain deps> > sorted_deps.insert(hard_deps); > sort(sorted_deps) > > This will prevent the extra set construction just to merge with extra_hard_deps. Nice approach, and probably scales better too. Pretty similar base runtime, however. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:221: // This just writs all toolchain deps for simplicity. If we find that On 2015/12/07 15:09:50, M-A Ruel wrote: > writes > (while at it) Done. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_toolcha... File tools/gn/ninja_toolchain_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_toolcha... tools/gn/ninja_toolchain_writer.cc:65: // Sort targets so that they are in a deterministic order. On 2015/12/07 at 15:09:50, M-A Ruel wrote: > I'd still prefer: > DCHECK( > std::is_sorted(targets.begin(), targets.end(), > [](const Target* a, const Target* b) { return a->label() < b->label(); })); > > remove the const and enforce caller to fix their code. > > But in fact you really don't need to change this file at all, see the other comment. I've followed your advice here and effectively removed all changes to this file. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.cc File tools/gn/ninja_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:35: // Sort targets so that they are in a deterministic order. On 2015/12/07 at 15:09:50, M-A Ruel wrote: > I'd prefer WriteToolchains() to return a sorted list instead. This is a good idea. It'll improve speed (as the work was currently duplicated). https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:58: typedef std::map<Label, std::vector<const Target*> > CategorizedMap; On 2015/12/07 at 15:09:50, M-A Ruel wrote: > You could use a set<> for Target* here, so they would be sorted on the fly. Then the following iterations can stay const. > > wdyt? A std::set<Target*> would be sorted by pointer order, which is still nondeterministic. We could add a custom comparator to the set, which would probably make that approach better, /but/ subsequent functions expect a std::vector. I've reshuffled it slightly so that we have consts more often, however. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:80: for (CategorizedMap::iterator i = categorized.begin(); i != categorized.end(); On 2015/12/07 at 15:09:50, M-A Ruel wrote: > If you use a set above, you can switch to an enumeration using "const auto&" Have done.
This lgtm but Brett makes the actual decision. https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.cc File tools/gn/ninja_writer.cc (right): https://codereview.chromium.org/1494883002/diff/120001/tools/gn/ninja_writer.... tools/gn/ninja_writer.cc:58: typedef std::map<Label, std::vector<const Target*> > CategorizedMap; On 2015/12/08 09:40:42, Zachary Forman wrote: > On 2015/12/07 at 15:09:50, M-A Ruel wrote: > > You could use a set<> for Target* here, so they would be sorted on the fly. > Then the following iterations can stay const. > > > > wdyt? > > A std::set<Target*> would be sorted by pointer order, which is still > nondeterministic. > > We could add a custom comparator to the set, which would probably make that > approach better, /but/ subsequent functions expect a std::vector. > > I've reshuffled it slightly so that we have consts more often, however. I had meant with a std:less<> that short based on the target name, but it's not a big deal. https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... File tools/gn/ninja_target_writer.cc (right): https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:217: for (const Target* target : extra_hard_deps) { > In this case, the extra_hard_deps and toolchain_deps are almost always empty > and if they aren't, they will be small. In contrast, the recursive_hard_deps can > be several hundred items. I think this comment should be added here, it's not obvious (at least to me) and it's important to understand the performance characteristics. https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:229: if (!hard_deps.count(toolchain_dep.ptr)) { I guess this assumes toolchains are never passed as extra_hard_deps ?
LGTM, thanks! Just some minor nits before landing... https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... File tools/gn/ninja_target_writer.cc (right): https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:210: // them. Note that the order matters, or the output is non-deterministic. Can you rephrase this comment, I feel like it's worded too strongly and implies that non-ordered might be wrong whereas this is more of a nice-to-have. Maybe just "These are sorted so the generated files are deterministic." https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:218: if (!hard_deps.count(target)) { No {} for single line conditionals (for consistency with the rest of the code in this file). Same thing in a few cases below. https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... tools/gn/ninja_target_writer.cc:229: if (!hard_deps.count(toolchain_dep.ptr)) { On 2015/12/08 13:49:29, M-A Ruel wrote: > I guess this assumes toolchains are never passed as extra_hard_deps ? You mean "toolchain deps" (these are target dependencies required by everything in a toolchain, designed for nacl toolchain setup steps). In our build this should never be the case, but in general this is not guaranteed. I think this isn't worth worrying about since it's quite hard to come up with a legitimate reason why this might happen, and the failure mode is a harmless redundant dependency. And if we start getting interesting toolchain dependencies we should do the TODO already mentioned about the stamp file. However, we should add a comment to this effect here: "These could theoretically duplicate dependencies already in the list, but shouldn't happen in practice, is inconvenient to check for, and only results in harmless redundant dependencies listed."
On 2015/12/10 at 21:29:46, brettw wrote: > LGTM, thanks! Just some minor nits before landing... > > https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... > File tools/gn/ninja_target_writer.cc (right): > > https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... > tools/gn/ninja_target_writer.cc:210: // them. Note that the order matters, or the output is non-deterministic. > Can you rephrase this comment, I feel like it's worded too strongly and implies that non-ordered might be wrong whereas this is more of a nice-to-have. Maybe just "These are sorted so the generated files are deterministic." > > https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... > tools/gn/ninja_target_writer.cc:218: if (!hard_deps.count(target)) { > No {} for single line conditionals (for consistency with the rest of the code in this file). Same thing in a few cases below. > > https://codereview.chromium.org/1494883002/diff/160001/tools/gn/ninja_target_... > tools/gn/ninja_target_writer.cc:229: if (!hard_deps.count(toolchain_dep.ptr)) { > On 2015/12/08 13:49:29, M-A Ruel wrote: > > I guess this assumes toolchains are never passed as extra_hard_deps ? > > You mean "toolchain deps" (these are target dependencies required by everything in a toolchain, designed for nacl toolchain setup steps). > > In our build this should never be the case, but in general this is not guaranteed. I think this isn't worth worrying about since it's quite hard to come up with a legitimate reason why this might happen, and the failure mode is a harmless redundant dependency. And if we start getting interesting toolchain dependencies we should do the TODO already mentioned about the stamp file. > > However, we should add a comment to this effect here: "These could theoretically duplicate dependencies already in the list, but shouldn't happen in practice, is inconvenient to check for, and only results in harmless redundant dependencies listed." Done!
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/1494883002/#ps180001 (title: "Code delousing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494883002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494883002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from
==========
GN: Makes GN output deterministic
Sorts various output just prior to printing to ensure that GN output
is deterministic.
The overall effect on performance is relatively small, with
runtime over 50 runs varying as follows on my Z840:
+-------+-------+-------+
| | old | new |
+-------+-------+-------+
| mean | 1415 | 1430 |
+-------+-------+-------+
| stdev | 58.0 | 70.5 |
+-------+-------+-------+
To verify results:
$ gn gen out/Default; mv out out-1
$ gn gen out/Default; mv out out-2
$ diff -qr out-1 out-2
The diff should be empty.
Initial discussion can be seen at
https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI.
BUG=565075
==========
to
==========
GN: Makes GN output deterministic
Sorts various output just prior to printing to ensure that GN output
is deterministic.
The overall effect on performance is relatively small, with
runtime over 50 runs varying as follows on my Z840:
+-------+-------+-------+
| | old | new |
+-------+-------+-------+
| mean | 1415 | 1430 |
+-------+-------+-------+
| stdev | 58.0 | 70.5 |
+-------+-------+-------+
To verify results:
$ gn gen out/Default; mv out out-1
$ gn gen out/Default; mv out out-2
$ diff -qr out-1 out-2
The diff should be empty.
Initial discussion can be seen at
https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/8mOLgM4r3PI.
BUG=565075
Committed: https://crrev.com/98ec25a8fb3f710d43d60b1bcdc5a1475aab136f
Cr-Commit-Position: refs/heads/master@{#364954}
==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/98ec25a8fb3f710d43d60b1bcdc5a1475aab136f Cr-Commit-Position: refs/heads/master@{#364954} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
