New issue
Advanced search Search tips

Issue 870622 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 869348
issue 908463



Sign in to add a comment

do_generate_fontconfig_caches build target has non-deterministic output

Project Member Reported by tikuta@chromium.org, Aug 3

Issue description

Clean 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


 
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
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. 
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Is there any other works we need to do here?
Components: Build
Status: Fixed (was: Assigned)
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.
Blocking: 908463

Sign in to add a comment