[PATCH] D90121: clang-format: Add a consumer to diagnostics engine

Thu Oct 14 07:56:58 PDT 2021

MyDeveloperDay added a comment.

@thakis , At the back of my mind is those people who always say "Measure, Measure, Measure"...

This issue still plagues us, we constantly get bugs reported that come back to this.  (https://bugs.llvm.org/show_bug.cgi?id=52021,https://bugs.llvm.org/show_bug.cgi?id=42014)

So I decided I should look into the concerns with us fixing this issue.

I decided to do 3 tests:

- a) Build current tip of main
- b) Apply this FrontEnd fix
- c) Move TextDiagnosticPrinter (and dependencies) into lib/Basic

I took a look and in its simplest form moving TextDiagnosticPrinter.cpp to lib/Basic but also requires DiagnosticRender.cpp and TextDiagnostic.cpp to also move to lib/Basic too,

But DiagnosticRender.cpp has a dependency on lib/Edit so clang-format now needs that as a dependency, is that better/worse than having a dependency on lib/FrontEnd? (not sure why I don't have to provide "Edit" as a dependency when including FrontEnd!)

For each test, I want to check

- a) size of the executable produced (debug/release)
- b) number of build dependencies (from ninja after ninja clean)

The size of the executable I assume is a concern as this impacts the runtime of clang-format as the system loads the module,
The number of build dependencies comes down to how quickly developers working on clang-format can rebuild.

I'm not concerning myself with the final "link" time of clang-format, as its relatively trivial on a modern machine, and nothing in comparison to building from clean.

Firstly I think we should recognize that this "crash" against a read-only file IS a problem that most of us will see from time to time and at best
its annoying. So I think there is enough momentum for us to have a solution of one sort of another.

**Build speeds**

Build speeds are important if you are a LLVM developer as you don't really want to have to keep rebuilding, I personally use ninja on Windows
having given up using vs projects generated by cmake years ago as being way too slow in the first place.

Ninja will give me the number of "targets" needed to compile, rather than timing it, I'll simply work on the assumption that "less is best"

  $ ninja clang-format
  [698/1298] Building CXX object lib\Object\CMakeFiles\LLVMObject.dir\SymbolicFile.cpp.obj

Before building each of the targets I perform a "ninja clean" so that hopefully the build that follows shows me how many targets I needed

1. Using the current tip of master, the number of targets to build clang-format sits at 608
2. If I apply this FrontEnd patch D90121: clang-format: Add a consumer to diagnostics engine <https://reviews.llvm.org/D90121> it rises to `1352`

3 )If I apply a patch that moves just `TextDiagnosticPrinter/TextDiagnostic/DiagnosticRender` into `lib/Basic` then the number is `1298`

**Executable Size**

For Completeness I built both Debug and Release (mainly because I live in Debug as I work on clang-format, but the releases are Release (i assume))

**Debug (clang-format)**

For Debug there seems no benefit for moving the code out of FrontEnd into Basic

  -rwxr-xr-x 1  17425920 Oct 14 14:48 clang-format-frontend.exe   (Debug)
  -rwxr-xr-x 1  17425920 Oct 14 14:40 clang-format.basic.exe (Debug)
  -rwxr-xr-x 1  17171968 Oct 14 14:44 clang-format.original.exe (Debug)

The modified debug binaries are only 1.5% larger.

**Release (clang-format)**

For Release the Basic version was actually larger.

  -rwxr-xr-x 1   2601472 Oct 14 15:17 clang-format-release.basic.exe   (Release)
  -rwxr-xr-x 1   2600960 Oct 14 15:12 clang-format-release-frontend.exe (Release)
  -rwxr-xr-x 1   2041344 Oct 14 15:14 clang-format-release.orginal.exe (Release)

With both Basic and FrontEnd release binaries being `~27%` larger.

**Conclusion**

This bug is annoying.

1. I just don't think an approach that moves the files into Basic is the solution.

- a) It requires multiple CMakeFile changes
- b) We have to leave header stubs lying around (or fix all the other tools that might include them and that is ugly)
- c) The binary is actually larger (go figure!)
- d) The number of files to be build is 213% as many as not having this fix, even if its ~10% less than the frontend fix (thats not worth the difference)

2. Frontend fix is the simplest solution (and has been sent for review at least 3 times that I can find)

- a) Its slightly annoying because I keep having to defend why we don't want to fix it!
- b) Its 222% more files during the build cycle (however we don't build clean every time when building clang-format) so for a clang-format developer the impact is likely minimal
- c) The binaries are larger, less than the basic but still ~27% larger (which one could see as causing a slowdown on startup, although I have not measured that)

I'm kind of the opinion that we should resolve the crash before worrying about binary size aspect. Like I said I primarily use debug builds of clang-format
even during my working day so that I am testing what we are developing and whilst they are slower because they are debug, I actually don't find them intolerable and they are almost `900%` larger.

I have yet to investigate if there is some other way. (like guarding the crash)

I feel we should take another look to see if we can't find a simpler alternative, but I personally don't find the build times or binaries sizes to be pervasive to starting with this as a fix.

Could we discuss it again given that it continues to be an annoyance, keeps getting reported, has a simple enough initial fix, doesn't seem to have a terrible detrimental impact.

MyDeveloperDay.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90121/new/

https://reviews.llvm.org/D90121