Repository navigation
Take no .init of the inspected type in traits - #11119
ehudt-weka wants to merge 1 commit into
Conversation
|
Thanks! |
| of type `T`. They now use $(REF lvalueOf, std, traits) and $(REF rvalueOf, std, traits), | ||
| so their compile-time memory and time no longer grow with the size of `T`. | ||
|
|
||
| Answers are unchanged, except in these cases: |
There was a problem hiding this comment.
could you split these cases that change out into a separate PR so that is obvious as to what they are? Also that would enable us to merge the cases that don't change much sooner.
There was a problem hiding this comment.
Done. The cases that change answers are now in #11143, stacked on this PR: each with a ref parameter on a forward range, ElementType of an enum whose base type is an array, and element types with a member named init.
This PR keeps every answer except for a type that declares a member named init. There T.init named the member, so the traits answered for the member instead of the type. Keeping those answers would need a guard at about 30 places, so the changelog documents the change instead and a new unittest covers it. I can add the guards if you prefer.
While splitting I found that the earlier comparison had stopped at dmd's 20-error limit; the description now gives the full numbers.
For a struct or a static array, `T.init` makes the compiler build the type's whole default-initializer literal, one entry per static-array element. The range primitives traits, std.traits (CommonType, isIterable, ForeachType, hasElaborateAssign, the *TypeOf alias-this traits, ...), and constraints in std.algorithm, std.array and std.typecons used it only to form an expression of type `T`. They now use lvalueOf and rvalueOf, so their compile-time memory and time no longer grow with the size of `T`. CommonType keeps `.init` for operands that are not a struct or union, since null and array literals merge differently in `?:` than values of the type do. Answers are unchanged, except for a type that declares a member named `init`: `T.init` named that member, so the traits answered for the member instead of the type.
|
So, this is supposed to help speed up compilation, because it avoids needing to calculate |
ae4b0ab to
79df48d
Compare
|
To see how much the compiler could avoid, I prototyped rikkimax's suggestion from #11143: dmd skips the literal when The extra template instances do cost something, and the description measures it: compiling Phobos (the dmd perf-check workload) peaks 0.9% higher in memory, with no change in time. On a struct holding a |
…683364 Brings in the review fixes of dlang#11119 and dlang#11143. `ifTestable` on a struct or union: when `pred` does not take a `T`, try an rvalue of each other qualifier of `T`, since the unique literal `T.init` converted to all of them. A struct with indirections and a `const` or `immutable` predicate parameter is testable again, as before this change. Tests for `isIterable`/`ForeachType` (`ref @safe` opApply, non-copyable range), the range traits without `@property`, a member named `init`, an enum based on an array, `each` with a `ref` parameter on a field `front`, `Chunks` over an infinite range in `@safe pure` code, and qualified `isTuple`. Comments reworded.
The the compiler should probably be changed to cache the result. I really don't think that this is a desirable direction to go in. Maybe it makes sense for some stuff in Phobos in the short term if the performance difference is large enough, but this is something that affects far more than Phobos code, and I really don't think that we want to start telling everyone to use |
Any solution that requires solving D's technical debt is not a feasible one. Work around it and move on. |
I do not buy that, and a big part of the problem is this is not the sort of thing where you move on. If the problem is big enough to solve here, then it's big enough that it needs to be solved generally, and if we have to work around the compiler, that means that code in general is going to need to use |
Agreed. This is solvable; solve it properly. |
Moving on is how it will be solved. The same way "instantiating lots of templates is bad" was solved. Feel free to prove me wrong, though! |
|
I tried caching. Caching whole literals runs into CTFE and the optimizer changing literal nodes in place, but the costly part can be shared: every element slot of a static array's default-init literal is null (it stands for Peak memory and time on a struct holding
So I'd like to open the cache as a dmd PR; it helps every |
|
That is still a decent decrease, and a good example of why we are pushing for optimizing dmd first. If done sufficiently, this PR should decrease in size. |
Rationale
For a struct or a static array, an expression
T.initmakes the compiler build the type'swhole default-initializer literal, with one element expression per static-array element. Many
traits write
R.init.front,S.init.opAssign(...)orforeach (e; T.init)only to get anexpression of type
T, so they cost memory and time in proportion to the size ofT. On astruct with a
ubyte[1 << 20]field, the range primitives and a dozenstd.traitsqueriestake 0.29 s and 329 MiB, against 0.09 s and 65 MiB with this PR.
map,minElement,appenderand others pass the same costs on to user code through their constraints.lvalueOf!TandrvalueOf!Tfromstd.traitsare declared, never-defined functions thatgive an expression of type
Twithout building a literal. This PR uses them wherever.initof the inspected type served only that purpose:
std.range.primitives:isInputRange,isForwardRange,isBidirectionalRange,hasSlicing,ElementType(and through itElementEncodingTypeand the other traits builton it).
std.traits:CommonType,isIterable,ForeachType,hasElaborateAssign,hasNested,fullyQualifiedName(static arrays),ifTestable, and the alias-this lookup inBooleanTypeOf,IntegralTypeOf,FloatingPointTypeOf,CharTypeOf,StaticArrayTypeOf,AssocArrayTypeOfandBuiltinTypeOf.std.algorithm: the constraints and checks ofmap,substituteandminElement/maxElement(extremum).std.array:Appender'scanPutRangeandcanPutConstRange.std.typecons:isTuple,areCompatibleTuples,Tuple._Tuple_super,Rebindable,Rebindable2, andapply.As asked in review, this PR keeps the answers of these traits, with one exception, below. The
.inituses whose replacement changes answers are in #11143, stacked on this one.The non-obvious choices:
isInputRangeandElementTypealso requireis(typeof(R.sizeof)).R.initused to laythe struct out first. Without that step, a range trait evaluated while the struct's own
declaration is still being analysed infers the struct's member functions as impure and
@system. One example is a trait in a member's return type, as inChunks.opSlice, whichreturns
typeof(takeExactly(this, 0)). In that casesequence!"..."(1, 2).chunks(2)stopscompiling in
@safe purecode. A new unittest covers this case.ElementTypeandisBidirectionalRangestill take the.initof the element type(
lvalueOf!R.front.init), andElementTypekeepsR.init.front.initfor an enum. Anenum's
.initis its first member, not a new literal, and only that expression gives anenum whose base type is an array a
front. Replacing either changes answers, so both are inTake no
.initof the element type in ElementType, nor of the range in each #11143.isIterableandForeachTypetryforeachover*pwithT* p = nullfirst, then overrvalueOf!T. AnopApplythat takes a@safedelegate refuses bothlvalueOf!TandrvalueOf!T: in a function literal whose attributes are inferred, the call to the bodylesshelper makes the loop body's delegate
@system. A non-copyable range needs the rvalue,because
foreachover*pwould copy it.CommonTypeswitches only struct and union operands. Those are where the large literalsare. For other types,
.initis a literal such asnull,[]or0, and replacing itchanges answers that rely on literal conversions.
ifTestablekeepsT.initfor types other than structs and unions, for the same reason:ifTestable!(int, (ubyte a) => a)must stay true by value range propagation. For a structor union it tests
pred(rvalueOf!T). When that does not compile, it tests an rvalue of eachqualified form of
Unqual!T(mutable,const,immutable,shared,shared const). Theliteral
T.initis unique, so it converted to any of them, and master answered true whenpredtook any one.isTuplefirst checksis(Unqual!T == Tuple!Specs, Specs...). The literalT.initof aqualified tuple with indirections, such as
const Tuple!(int[], int), converted to themutable
Tuple!Specsparameter of the probe function as a unique expression;rvalueOf!Tdoes not.
Changed answers
A differential check printed the answer of every changed trait, and whether each changed
function accepts the type, for 629 types: 138 types in up to 5 qualifier variants each. The
types include structs with and without indirections, destructors, postblits, copy
constructors, disabled copy, default construction and assignment, alias this to every builtin
kind, unions, enums with every kind of base type, classes, tuples,
Nullable, arrays ofstructs, and ranges with field, method, property and
refprimitives. Of 64,860 answers, 559differ between master and this PR, in two groups:
init(554 answers, 6 types in 5 qualifier variants).There,
T.initnamed that member, so the traits answered for the member, not the type.For example, a range type with a member function
initwas not an input range on master,and is one now. Keeping master's answers would need a guard for such types at about 30
places, so this PR does not, and the changelog entry documents the change. A new unittest
covers it; dmd accepts such a member declared directly, and the test declares it through a
mixin template only because D-Scanner flags a member named
init.shared conststruct withalias thisto a static array, master's answers depend onwhich queries on the same type came first (5 answers). After the other queries in the
check,
ElementTypeandElementEncodingTypearevoid,isIterableis false,ForeachTypeis an error, andeachrefuses the type. Queried alone, each gives the sameanswer as this PR:
shared(const(int)),true, andeachaccepts it.An earlier revision of this description reported 101 of 50,550 answers changed. That run of
the check stopped at dmd's default limit of 20 errors, so about 20 types were never compared.
This run used
-verrors=0.Measurements
The same x86_64 dmd (7023163b4) and the same druntime imports were used for both trees
(macOS,
-o-unless noted, peak RSS from/usr/bin/time -l, 3 runs each, median time).These numbers replace the ones in the earlier description, which I could not reproduce for
the Phobos queries alone.
Large types. The range primitives,
CommonType,isIterable,ForeachType,fullyQualifiedName,hasNested,ifTestable,IntegralTypeOfandisTuplewere queriedon structs holding a
ubyte[N], and on a bidirectional range holding one:Most of what still grows with N here is
ElementTypeandisBidirectionalRangeonHuge[],which take the
.initof the element type; with #11143 the last row is 0.10 s, 54 MiB.Functions on such a struct (N = 2^22), one function per compile:
a.map!(e => e.x).suma.each!((ref e) => e.x++)on a static arrayminElement!(e => e.x)+maxElement!(e => e.x)appender!(Huge[])putof an element and of a sliceNullable!Huge.applyRebindable!(const Huge)sumover the rangeNullablebuildsDontCallDestructorT.initas a field initializer, which costs the same onboth trees. The rest of what remains comes from
ElementType(see above) and, forminElement/maxElement, from function bodies, which I have not traced.Ordinary code. Peak RSS rises slightly. Time does not change beyond run-to-run noise.
dmd -c -i=std -preview=dip1000 std/package.d, the dmd perf-check workload):580–583 MiB before, 587–588 MiB after (+0.9%).
ctRegexunittests (std/regex/internal/tests.d): 2053 MiB before, 2052–2056 MiB after.-unittest -o-of 16 Phobos modules: +0.5% (std.regex) to +2.6%(
std.algorithm.iteration), median +1.2%. Four of these modules also gain this PR's newunittests.
The increase comes from template instances:
lvalueOf!RandrvalueOf!Rare one instanceper inspected type, which costs more than
R.initwhenRis small.Testing
chunksofsequencein a@safe pureunittest. This guards theR.sizeofstep above:it passes on master, and fails on this PR without
R.sizeof.@propertyprimitive.isIterableandForeachTypeon anopApplywith aref@safedelegate and on anon-copyable range.
isTupleon qualified tuples with indirections.isInputRangeandisForwardRangeon a range type with a member functioninit(fails on master).
make unittest(release, x86_64) on macOS, where dmd runs under Rosetta 2, at thisrevision and at master a17028b, both against the druntime of dmd 7023163b4: 119 modules
pass and 10 fail, the same modules on both, and all touched modules pass. Nine failures are
in floating-point code, which goes wrong under Rosetta's x87 emulation (
std.math.*,std.mathspecial,std.numeric,std.internal.math.*,std.format.internal.floats,std.conv);std.pathruns out of memory. For the same reason the unittests of
std.format.internal.writeandstd.math.exponential,.operationsand.traitsdo not compile in this environment, onmaster too; those four were built without
-unittest.Pre-review checklist
LLM/AI disclosure
This PR was written entirely by an LLM/AI.
An AI coding agent (Claude, by Anthropic) wrote the code, the tests and this description, ran
the checks and measurements in it, and opened this PR at @ehudt-weka's request, from that
account; "I" above is the agent. The self-review checked above is the agent's own (the tests,
the differential check and the measurements), not a human code review.