do_generate_fontconfig_caches build target has non-deterministic output |
||||
Issue descriptionClean build of do_generate_fontconfig_caches generates non-deterministic file under fontconfig_caches dir. The file becomes different in both name and its content. e.g. fontconfig_caches/9baca54c-7820-4e37-a380-b2fc48fc4bbe-le64.cache-7
,
Aug 3
FcInit() [from third_party/fontconfig] generates non-deterministic output. Specifically - it generates a UUID to identifier test_fonts, see: https://cs.chromium.org/chromium/src/third_party/fontconfig/src/src/fccache.c?l=104 This is the only difference in the test_fonts directory from two runs of FcInit. """ $ diff out/gn/test_fonts ./tmp3/test_fonts/ diff out/gn/test_fonts/.uuid ./tmp3/test_fonts/.uuid 1c1 < 66cf4d6d-6e6e-4a79-af55-0b6f75fc780f \ No newline at end of file --- > d0eccd0f-6e7b-42a0-95ae-4300920be4cc \ No newline at end of file """ This then carries over to a difference in fontconfig_caches -- the name of the cache file is based on this UUID. This logic was recently added: https://chromium-review.googlesource.com/c/chromium/src/+/938578. Weirdly enough, the CL in question adds "fontconfig_caches" as a data dependency, but not "test_fonts". This doesn't make sense to me -- bug? According to the CL description: """ Additionally, building the fontconfig cache is a nontrivial task. It can take ~600ms when done from scratch. To fix this, fontconfig cache files are saved to the out directory. Fontconfig initialization takes < 1ms with this optimization, and the initialization only needs to happen once per test suite, not at the beginning of each test. Finally, to prevent the 600ms from being added to the first test suite to run, the cache files are generated as part of the build. """ So this was added to save ~600ms [since the FcInit cache is shared between test suites]. In general, when balancing test time vs compile time, I think we should aim to minimize compile time, since tests can be deduplicated via swarming.
,
Aug 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a48130cf3fda1415c35c39e786f444989b06a839 commit a48130cf3fda1415c35c39e786f444989b06a839 Author: Erik Chen <erikchen@chromium.org> Date: Sat Aug 04 00:27:18 2018 Make fontconfig cache construction deterministic...for now. We currently front-load construction of the font cache [which takes 600ms] from test run time to compile time. This saves 600ms on each test shard which uses the font cache. The problem is that fontconfig cache construction is not intended to be deterministic. This CL sets some external state to ensure deterministic output. We have no way of guaranteeing that this produces correct results, or even has the intended effect. I personally think we should get rid of this step entirely. The time saved per test shard is not worth the developer time it takes to try to make fontconfig cache output deterministic. This CL makes two changes: * Pregenerates a fixed uuid in the test_fonts directory. * Sets the mtime of the test_fonts directory to a date far in the past. This CL also fixes the gn steps to correctly specify data outputs so the isolate will actually pick up all the output. Future rolls of fontconfig may break build determinism again. Hopefully https://bugs.chromium.org/p/chromium/issues/detail?id=870731 will be fixed by then so that this will be caught by the CQ/waterfall. Change-Id: I89180cd3645ced2f83513c796195977f865df42c Bug: 870622 Reviewed-on: https://chromium-review.googlesource.com/1161969 Commit-Queue: Erik Chen <erikchen@chromium.org> Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Takuto Ikuta <tikuta@chromium.org> Cr-Commit-Position: refs/heads/master@{#580715} [modify] https://crrev.com/a48130cf3fda1415c35c39e786f444989b06a839/base/test/BUILD.gn [modify] https://crrev.com/a48130cf3fda1415c35c39e786f444989b06a839/base/test/generate_fontconfig_caches.cc
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/490a1c4348fc61ea3364a89cb5c46db6bdf445c5 commit 490a1c4348fc61ea3364a89cb5c46db6bdf445c5 Author: Takuto Ikuta <tikuta@chromium.org> Date: Tue Aug 07 00:39:05 2018 Specify output files of do_generate_fontconfig_caches explicitly This does not allow arbitrary files are included in isolated input from fontconfig_caches directory. Bug: 870622 Change-Id: I69bc8993800f263d7e3d7679ed85f9c79a380a0b Reviewed-on: https://chromium-review.googlesource.com/1163596 Reviewed-by: Thomas Anderson <thomasanderson@chromium.org> Reviewed-by: Erik Chen <erikchen@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Takuto Ikuta <tikuta@chromium.org> Cr-Commit-Position: refs/heads/master@{#581056} [modify] https://crrev.com/490a1c4348fc61ea3364a89cb5c46db6bdf445c5/base/test/BUILD.gn [modify] https://crrev.com/490a1c4348fc61ea3364a89cb5c46db6bdf445c5/base/test/generate_fontconfig_caches.cc [modify] https://crrev.com/490a1c4348fc61ea3364a89cb5c46db6bdf445c5/third_party/test_fonts/README.chromium
,
Aug 8
Is there any other works we need to do here?
,
Aug 8
,
Aug 8
I believe this is done, but without a more comprehensive compare_build_artifacts.py, it's hard to be sure, and we won't have regression detection.
,
Nov 26
|
||||
►
Sign in to add a comment |
||||
Comment 1 by erikc...@chromium.org
, Aug 3Status: Assigned (was: Untriaged)