Page MenuHomePhabricator

[PDB] Enable parallel ghash type merging by default
ClosedPublic

Authored by rnk on May 20 2021, 4:35 PM.

Details

Summary

Ghashing is probably going to be faster in most cases, even without
precomputed ghashes in object files.

Here is my table of results linking clang.pdb:

threadsGHASHNOGHASH
j151.031s25.141s
j231.079s22.109s
j418.609s23.156s
j811.938s21.984s
j288.375s18.391s

This shows that ghashing is faster if at least four cores are available.
This may make the linker slower if most cores are busy in the middle of
a build, but in that case, the linker probably isn't on the critical
path of the build. Incremental build performance is arguably more
important than highly contended batch build link performance.

The -time output indicates that ghash computation is the dominant
factor:

  Input File Reading:             924 ms (  1.8%)
  GC:                             689 ms (  1.3%)
  ICF:                            527 ms (  1.0%)
  Code Layout:                    414 ms (  0.8%)
  Commit Output File:              24 ms (  0.0%)
  PDB Emission (Cumulative):    49938 ms ( 94.8%)
    Add Objects:                46783 ms ( 88.8%)
      Global Type Hashing:      38983 ms ( 74.0%)
      GHash Type Merging:        5640 ms ( 10.7%)
      Symbol Merging:            2154 ms (  4.1%)
    Publics Stream Layout:        188 ms (  0.4%)
    TPI Stream Layout:             18 ms (  0.0%)
    Commit to Disk:              2818 ms (  5.4%)
--------------------------------------------------
Total Link Time:                52669 ms (100.0%)

We can speed that up with a faster content hash (not SHA1).

Depends on D102885

Diff Detail

Event Timeline

rnk requested review of this revision.May 20 2021, 4:35 PM
rnk created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2021, 4:35 PM
rnk edited the summary of this revision. (Show Details)May 20 2021, 4:49 PM
rnk edited the summary of this revision. (Show Details)

We can speed that up with a faster content hash (not SHA1).

Definitely. xxHash in the LLVM tree gives quite good results, see https://reviews.llvm.org/D55585#1354878 Probably integrating the latest version would improve the figures (also it supports hardware vector instructions). https://github.com/Cyan4973/xxHash

lld/test/COFF/pdb-type-server-simple.test
23

I must confess I intuitively like better -debug:noghash because it's searchable & unique, and it's harder to spot the 'minus' in a large block of text. But there are maybe arguments both ways? :)

rnk updated this revision to Diff 346896.May 20 2021, 5:08 PM
  • use /debug:noghash
rnk added inline comments.May 20 2021, 5:09 PM
lld/test/COFF/pdb-type-server-simple.test
23

Yeah, I guess I agree.

aganea accepted this revision.May 20 2021, 5:34 PM
aganea added subscribers: mstorsjo, thakis.

LGTM. But perhaps @thakis and @mstorsjo might want to take a second look?

This revision is now accepted and ready to land.May 20 2021, 5:34 PM

LGTM. But perhaps @thakis and @mstorsjo might want to take a second look?

Sorry, I have little to no clue about PDB things, so I can't really give any meningful comment on this - but the patch overall looks reasonable.

Sorry, I have little to no clue about PDB things, so I can't really give any meningful comment on this - but the patch overall looks reasonable.

Sorry I should have been more specific: I was wondering if you had an opinion for -debug:ghash- vs. -debug:noghash.

@rnk as a next step, you would probably want to re-do D43881 so that -gcodeview-ghash is enabled by default when building LLVM?

rnk added a comment.May 21 2021, 8:21 AM

@rnk as a next step, you would probably want to re-do D43881 so that -gcodeview-ghash is enabled by default when building LLVM?

Oh right, I guess I switched to gn on Windows since then. You can see I did the equivalent here:

$ git grep -B1 gcodeview-ghash llvm
llvm/utils/gn/build/BUILD.gn-      if (use_lld && is_clang) {
llvm/utils/gn/build/BUILD.gn:        cflags += [ "-gcodeview-ghash" ]

I think at the time that I was using cmake, I just kept adding -gcodeview-ghash to CMAKE_CXX_FLAGS.

This revision was automatically updated to reflect the committed changes.
rnk added a comment.Thu, May 27, 2:38 PM

I suppose I should've updated D43881, but I made a new one at https://reviews.llvm.org/D103287. Oh well.