Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit struct TypeInfos in referencing compilation units only #3491

Merged
merged 3 commits into from
Oct 24, 2020

Conversation

kinke
Copy link
Member

@kinke kinke commented Jul 3, 2020

First commit only:

Analogous to ClassInfos, incl. normal linkage (external for non-templates, weak_odr for templates).

This enables to get rid of frontend logic wrt. whether to add TypeInfoStructDeclarations to a module's members tree - previously,
it was defined as linkonce_odr in the owning module and each referencing module (unless speculative) - and related extra semantic and codegen for the special member functions.
I've gone a bit further and moved the entire TypeInfo emission for LDC to the codegen layer; no TypeInfoDeclarations are added to the module members anymore. Whenever we need a TypeInfo symbol during codegen, it is declared or defined, and we don't need to rely on brittle frontend logic with speculative-ness complications.

This might slightly increase compilation speed due to less emitted TypeInfos and functions (possibly less work for the linker too).

It might require LTO to avoid performance regressions though and delegates the job of stripping unused struct TypeInfos to the linker, as the TypeInfo is guaranteed to end up in the owning object file due to no linkonce_odr.

Re-emitting the struct TypeInfos (and optionally the special member functions too) into each referencing CU could be handled in our codegen layer, which should be much simpler and more robust than the upstream scheme.

@kinke kinke force-pushed the iraggr2 branch 2 times, most recently from 7f842d9 to cb6c3de Compare July 3, 2020 22:15
@kinke kinke changed the title Only emit struct TypeInfos & special members in owning module Only emit struct TypeInfos & special member functions in owning module Jul 3, 2020
@kinke
Copy link
Member Author

kinke commented Jul 3, 2020

Thoughts and/or compile/runtime/size numbers would be appreciated.

@kinke kinke marked this pull request as ready for review July 3, 2020 22:43
@kinke
Copy link
Member Author

kinke commented Jul 3, 2020

@JohanEngelen: This also eliminates some needsCodegen() calls (e.g., in disabled isSpeculativeType()).

@kinke kinke changed the title Only emit struct TypeInfos & special member functions in owning module Emit struct TypeInfos & special member functions in owning module only Jul 4, 2020
@JohanEngelen
Copy link
Member

@kinke It will be a while before I can look at this and test it , sorry

It might require LTO to avoid performance regressions

What specific type of perf regression do you expect?

@kinke
Copy link
Member Author

kinke commented Jul 4, 2020

I'm in no hurry wrt. this, so take your time. :)

What specific type of perf regression do you expect?

I was thinking about cases where the optimizer might have been able to inline xtoHash/xopEquals/xopCmp/xtoString/xdtor[ti]/xpostblit indirections. But after looking at TypeInfo_Struct, these (public) members are probably hardly used directly, and instead used by the virtual TypeInfo_Struct methods equals/compare/getHash/destroy/postblit, which are in druntime's object.o object file only (incl. the TypeInfo_Struct vtable), so that's probably not an issue at all.

@kinke
Copy link
Member Author

kinke commented Aug 20, 2020

Okay to merge? The runtime performance concerns shouldn't apply as mentioned above; size-wise, I've noticed some slight increases for built executables, but nothing dramatic IIRC. - It's a prerequisite for a clean reboot of #3422.

@kinke
Copy link
Member Author

kinke commented Sep 1, 2020

This does have quite an impact - some binary size numbers (in bytes) for Win64, based on the Azure packages:

master this
bin dir (self-hosted) 130,823,680 130,816,512 (-0%)
static druntime debug 9,873,312 9,859,848 (-0.1%)
static druntime 4,498,708 5,617,442 (+24.9%)
static Phobos debug 18,536,984 18,477,206 (-0.3%)
static Phobos 8,916,078 9,155,182 (+2.7%)

So e.g. apparently lots of not-directly-used (in the same CU) struct TypeInfos in druntime which aren't stripped anymore with enabled optimizations (but e.g. also aren't emitted in user-code and Phobos CUs anymore if the structs aren't inside templates) and need to be stripped by the linker.

@kinke
Copy link
Member Author

kinke commented Sep 1, 2020

And the debug lib sizes probably show that at least for druntime and Phobos, there are hardly any extra emissions in non-owning CUs.

Maybe we should instead strive for emitting them only in referencing CUs as linkonce_odr, not in the owning one at all if not referenced therein. At least the TypeInfos themselves, not sure about the special member functions.

@JohanEngelen
Copy link
Member

Okay to merge? The runtime performance concerns shouldn't apply as mentioned above; size-wise, I've noticed some slight increases for built executables, but nothing dramatic IIRC. - It's a prerequisite for a clean reboot of #3422.

I'll try this on weka's code this week.

@kinke kinke force-pushed the iraggr2 branch 2 times, most recently from 8250244 to fbb03c7 Compare September 1, 2020 18:15
@kinke
Copy link
Member Author

kinke commented Sep 1, 2020

Wrt. the 2nd approach in the 2nd commit, I don't have a good idea wrt. the missing dtor linker failure in runnable\link15017.d, and why it works for Linux and FreeBSD but doesn't work for Windows and Mac. But I haven't looked at it in detail yet. Anyway, the numbers are quite interesting (percentages relative to master again):

this (2nd commit)
static druntime debug 8,746,420 (-11.4%)
static druntime 4,514,284 (+0.3%)
static Phobos debug 18,241,660 (-1.6%)
static Phobos 8,948,460 (+0.4%)

@kinke
Copy link
Member Author

kinke commented Sep 9, 2020

I don't have a good idea wrt. the missing dtor linker failure in runnable\link15017.d

Looked at it (on Windows), and for some bizarre reason, the extra semanticTypeInfoMembers() call leads to the (already sema3'd!) dtor losing the pure @safe attributes (and thus a wrong mangled name accounting for the linker error)...

@JohanEngelen
Copy link
Member

JohanEngelen commented Sep 9, 2020

No linking errors at Weka, good news. Attribute deduction problems (the pure @safe problem you mention) are very common at Weka after dlang frontend changes, so it's good news that this didn't introduce any for that codebase.

Some compilation size data from Weka:

 Before:        After:
   2338208     2223504
   1196240     1082032
  45730696    44155640
    178768      155232
     59984       39816
   3738096     3660928
   2100416     2024160
    916296      884192
   1172320     1151992
    557808      496680
    448144      386880
  10752160    10269976
1005558456  1004152488   # yes the unoptimized binary is 1 GB.
    314144       72432

That's a net size reduction across the board. Measuring compilation time will be very flawed, but at least there was no obvious slow down / speed up.

@kinke
Copy link
Member Author

kinke commented Sep 10, 2020

Thx for the numbers! [And I'm surprised there were no linker errors...]

Upstream, Walter and Andrei are working on getting rid of these compiler-built TypeInfos (struct TypeInfos at least) and replacing them by straight-forward templates in druntime. So I'm not sure how long that takes and whether getting to the bottom of this weird attributes-inference problem in the meantime is worth it.

kinke added 3 commits October 11, 2020 12:58
Analogous to ClassInfos, incl. normal linkage (external for non-
templates, weak_odr for templates).

This enables to get rid of frontend logic wrt. whether to add
TypeInfoStructDeclarations to a module's members tree - previously,
it was defined as linkonce_odr in the owning module and each referencing
module (unless speculative) - and related extra semantic and codegen for
the special member functions.
I've gone a bit further and moved the entire TypeInfo emission for LDC
to the codegen layer; no TypeInfoDeclarations are added to the module
members anymore. Whenever we need a TypeInfo symbol during codegen, it
is declared or defined, and we don't need to rely on brittle frontend
logic with speculative-ness complications.

This might slightly increase compilation speed due to less emitted
TypeInfos and functions (possibly less work for the linker too).

A slight drawback is that the job of stripping unused struct TypeInfos
is fully delegated to the linker, as the TypeInfo is guaranteed to end
up in the owning object file due to no linkonce_odr.

Another theoretical drawback is that the optimizer can definitely not
inline xtoHash/xopEquals/xopCmp/xtoString/xdtor[ti]/xpostblit function
pointer indirections in non-owning CUs without LTO (neither the pointers
nor the special member functions are defined anymore).
These (public) members are probably hardly used directly though, and
instead used by the virtual TypeInfo_Struct methods equals/compare/
getHash/destroy/postblit, which are exclusively defined in druntime's
object.o (incl. the TypeInfo_Struct vtable) and aren't cross-module-
inlined anyway (without LTO).

Re-emitting the struct TypeInfos (and optionally the special member
functions too) into each referencing CU could be handled in our codegen
layer, which should be much simpler and more robust than the upstream
scheme.
I.e., not in their owning CU if unused therein, but in all referencing
CUs instead, as linkonce_odr. The special member functions are still
only and unconditionally emitted in the owning CU.
This fixes dmd-testsuite's runnable/link15017.d, where the dtor checks
were performed before it had been semantically analyzed, leading to it
wrongly being marked as impure and non-safe, and thus a later linker
error.
@kinke
Copy link
Member Author

kinke commented Oct 11, 2020

Found the reason for the failure; it was (deprecated) delete specific (a general front-end issue apparently), thus not really surprising there weren't any linker issues for Weka.

@kinke
Copy link
Member Author

kinke commented Oct 11, 2020

To elaborate a bit: when a function hasn't had its attributes inferred yet, but is checked for being pure/safe/..., it is set as impure/non-safe (wtf?), and a later sema doesn't infer them anymore...

E.g., https://github.com/dlang/dmd/blob/b6526dfc9994049b386ab1da0e38d7655adedf73/src/dmd/func.d#L1337-L1338 and https://github.com/dlang/dmd/blob/b6526dfc9994049b386ab1da0e38d7655adedf73/src/dmd/func.d#L1376-L1381.

@kinke kinke changed the title Emit struct TypeInfos & special member functions in owning module only Emit struct TypeInfos in referencing compilation units only Oct 11, 2020
@JohanEngelen
Copy link
Member

To elaborate a bit: when a function hasn't had its attributes inferred yet, but is checked for being pure/safe/..., it is set as impure/non-safe (wtf?), and a later sema doesn't infer them anymore...

We generally do have linking errors related to attribute inference. Weka's codebase has a bunch of explicit attributes on templates to fix those.

@kinke
Copy link
Member Author

kinke commented Oct 11, 2020

That's why I elaborated a bit. The proposed upstream fix for this particular case is dlang/dmd#11854, maybe it sparks a deeper discussion about why things are being done this way.

@kinke kinke merged commit a74601f into ldc-developers:master Oct 24, 2020
@kinke kinke deleted the iraggr2 branch October 24, 2020 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants