[PATCH] D114275: [lld-macho] Improve LC_FUNCTION_STARTS test coverage (NFC)
Keith Smiley via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 29 11:37:29 PST 2022
keith added a comment.
No problem! I think we were good with that version of the change, but lmk if you have any thoughts and I can fix it up.
================
Comment at: lld/MachO/SyntheticSections.cpp:800
+
+ for (const InputFile *file : inputFiles) {
+ if (auto *objFile = dyn_cast<ObjFile>(file)) {
----------------
oontvoo wrote:
> oontvoo wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > int3 wrote:
> > > > > thevinster wrote:
> > > > > > keith wrote:
> > > > > > > oontvoo wrote:
> > > > > > > > keith wrote:
> > > > > > > > > oontvoo wrote:
> > > > > > > > > > Do you happen to have performance data for this change?
> > > > > > > > > > seems like it could take a hit here , esp. for cases where the number of input files is large - ie., most of our apps :)
> > > > > > > > > >
> > > > > > > > > > On the other hand, I agree debug experience is important...
> > > > > > > > > >
> > > > > > > > > I've haven't tested this on our large links yet so I'll have to check, I agree, I felt bad writing this
> > > > > > > > How would people feel about making this configurable? ( either via making up some new flag or grouping it into an existing one)
> > > > > > > Maybe -no-function-starts is enough for this level of configuration? We actually have that enabled today for our builds for binary size.
> > > > > > Hmm... I think we can parallelize this. This might offset the need to gate it behind a flag. Although, we can hold off on this until more concrete data on how bad this affects the link speed.
> > > > > >
> > > > > > Why can't we add these symbols to the symtab?
> > > > > > Why can't we add these symbols to the symtab?
> > > > >
> > > > > the symtab is for global (aka extern) symbols, which need to have unique names across all object files. local symbols do not have that constraint.
> > > > >
> > > > > Anyway, this is being done in parallel with other stuff (see `finalizeLinkEditSegment()`), so its perf impact might actually be negligible in practice
> > > > I would prefer we not add too many config flags. I'm also not sure there's much value in having partially emitted debug info -- I figure you either want or don't want it. `-no-function-starts` is probably enough for now.
> > > > I've haven't tested this on our large links yet
> > >
> > > We've been using chromium_framework as an ad-hoc benchmark, since it's something that all of us can repro: https://bugs.llvm.org/show_bug.cgi?id=48657#c0
> > >
> > > Would be nice to include some numbers for that in the commit message. For running the benchmark, I like to use https://github.com/asayers/cbdr/blob/master/cbdr.md
> > >
> > > My usual invocation is `cbdr sample "base:~/bench-base/ld64.lld @response.txt" "diff:~/bench-diff/ld64.lld @response.txt" --timeout=300s | tee results.csv | cbdr analyze -s 95`
> > SG 👍
> > > I've haven't tested this on our large links yet
> >
> > We've been using chromium_framework as an ad-hoc benchmark, since it's something that all of us can repro: https://bugs.llvm.org/show_bug.cgi?id=48657#c0
>
> To be frank, chromium is good for most benchmarking purposes - but it isn't quite representative enough for most of our internal apps (for one, it's much smaller). Unfortunately it's the only thing we have public so ... ¯\_(ツ)_/¯
>
>
> But anyways, I've run your diff on YT and int3 is right that there's negligible difference in link time.
>
>
> ```
> x ./LLD_YT_released_base.txt
> + ./LLD_TY_released_diff.txt
> N Min Max Median Avg Stddev
> x 5 13.33 13.72 13.56 13.534 0.15565989
> + 5 13.56 13.92 13.79 13.76 0.15636496
> No difference proven at 95.0% confidence
> (No diff in CPU/SYS time either)
> ```
>
> Sorry for the noise here :)
>
>
As far is perf is concerned, when linking the largest lyft ios app, we end up with ~1.1 million function starts and the time difference doesn't seem measurable with this when using this or passing -no_function_starts
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114275/new/
https://reviews.llvm.org/D114275
More information about the llvm-commits
mailing list