Closed Bug 1224450 (fast-compiledb) Opened 7 years ago Closed 7 years ago

Make the CompileDB backend work off data instead of recursing the make build system


(Firefox Build System :: General, defect)

Not set


(firefox47 fixed)

Tracking Status
firefox47 --- fixed


(Reporter: glandium, Unassigned)


(Blocks 2 open bugs)



(4 files)

The way the CompileDB backend works at the moment is that it essentially goes through all directories and does make showbuild for all of them, and this is very slow.

There are various things to overcome to make this possible, so this is likely to become a forest of bug dependencies. The goal is also not limited to the CompileDB backend: the real goal is to feed the FasterMake backend with everything needed to build C++, but making CompileDB faster is a good proxy for that because it forces to do all the things required to make the end goal actually happen.

I have done preliminary work (in the form of a very awful hack) that is able to generate, for a linux opt build, about 70% of the same command lines the current CompileDB backend generates. I will file dependent bugs according to my findings from this preliminary work.
Depends on: 1222321, 1222323, 1221453
Alias: fast-compiledb
Depends on: 1224452
Depends on: 1224460
Depends on: 1224463
Depends on: 1227380
Depends on: 1227384
Depends on: 1227385
Depends on: 1235733, 1235674, 1235675
Depends on: 1235738
Depends on: 1235743
Depends on: 1235748
The current status is that I'm able to generate 99.51% (2023 of 2033) of all target compile commands (so, no host compile) on a linux64 non-debug build. The remaining ones are:

- build/unix/elfhack/dummy.c
- build/unix/elfhack/test-array.c
- build/unix/elfhack/test-ctors.c
- $objdir/build/unix/elfhack/inject/x86_64.c
Those are actually tricky for multiple reasons, and I will ignore them for now.

- toolkit/xre/nsAppRunner.cpp
- toolkit/xre/nsEmbedFunctions.cpp
- toolkit/xre/ProfileReset.cpp
- $objdir/toolkit/xre/Unified_cpp_toolkit_xre0.cpp
All those add something like `-DTOOLKIT_EM_VERSION="46.0a1" -DGRE_BUILDID=20151230084421` through their Makefile.

- webapprt/gtk/webapprt.cpp
This one adds something like `-DGRE_BUILDID=20151230084421` through its Makefile. But it's set to go away soon, since webapprt is going to be removed.

- browser/app/nsBrowserApp.cpp
This one adds `-DPBMODE_ICO="../../dist/branding/pbmode.ico" -DNEWTAB_ICO="../../dist/branding/newtab.ico" -DNEWWINDOW_ICO="../../dist/branding/newwindow.ico" '-DDOCUMENT_ICO="../../dist/branding/document.ico" -DFIREFOX_ICO="../../dist/branding/firefox.ico"`, but those defines are actually not used for the C++ file, only for the RC file in the same directory.
Blocks: fastermake
Depends on: 1244997
Depends on: 1245013
Depends on: 1245015
Depends on: 1244999
Depends on: 1245022
Depends on: 1245027
Depends on: 1245035
Time for a status update: above 98% of target C, C++, ObjC, ObjC++ on Android, Mac and Linux. (no rust, and no host compilations ; the former is not currently supported by CompileDB anyways, and the latter is supported, but plenty wrong)
The closest to 100% is Android, where it's down to 4 files, all exclusively because of -DGRE_BUILDID.
(In reply to Mike Hommey [:glandium] from comment #3)
> The closest to 100% is Android, where it's down to 4 files, all exclusively
> because of -DGRE_BUILDID.

(with a yet-to-be-filed patch that I'm thoroughly testing on try because it removes a Makefile containing a workaround that I think is not necessary anymore, but needs douple checking in different configurations)
Depends on: 1245055
Depends on: 1245422
Depends on: 1245763
Blocks: 1224463
No longer depends on: 1224463
Depends on: 1245764
Depends on: 1246881
Depends on: 1247162
There are a few difficult directories to handle, with limited usefulness
compared to having the CompileDB properly filled for everything else
in a timely manner, so skip them for now.

Review commit:
See other reviews:
Attachment #8718093 - Flags: review?(gps)
The data is now sufficient to, with some convolution, generate
the same compilation database that recursing the tree with the showbuild
target does.

The resulting code is not the prettiest, and exposes the shortcomings of
the current data model. It is however a first step towards
fixing those shortcomings, because they are now more clearly identified.

This was validated on all platforms on try by checking the output of
  mach build-backend -b CompileDB -d -n
is empty when backing out the patch after running
  mach build-backend -b CompileDB

Review commit:
See other reviews:
Attachment #8718095 - Flags: review?(gps)
Attachment #8718092 - Flags: review?(gps) → review+
Comment on attachment 8718092 [details]
MozReview Request: Bug 1224450 - Ignore host compilations in the CompileDB backend. r=gps
Comment on attachment 8718093 [details]
MozReview Request: Bug 1224450 - Skip difficult directories in the CompileDB. r=gps

Attachment #8718093 - Flags: review?(gps) → review+
Comment on attachment 8718094 [details]
MozReview Request: Bug 1224450 - Ignore COMPILE_PDB_FLAG for the CompileDB. r=gps
Attachment #8718094 - Flags: review?(gps) → review+
Comment on attachment 8718095 [details]
MozReview Request: Bug 1224450 - Make the CompileDB derive its commands from the data. r=gps

Very, very nice.

It's really difficult to audit that all this behaves like all the logic in and However, verifying the output didn't change as a result of this patch is verification enough for me.
Attachment #8718095 - Flags: review?(gps) → review+
Blocks: 1247567
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.