[M] Change in dart/sdk[main]: [tfa] Prevent deferred constant accesses to be propagated
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
by 1.1% (both compressed & uncompressed)Did you look at why there was a size reduction?
Note that attributes you're clearing also carry inferred closure, in addition to the inferred constant.
Artificially limiting inference for deferred libraries looks suboptimal: why can't we infer closures so their calls can be analyzed and infer constants such as null/true/false so their values can be used to specialize code, perform better devirtualization etc? As usual, (not) inferring something has a ripple and non-local effect which may affect inference results in many other places.
Note that weaker inference may result in the smaller size because of the reduced devirtualization and reduced inlining. But it doesn't mean we should prefer weaker inference. Instead, we can be more smart about using (or not using) the results of the inference.
For constants in particular, I don't see dart2wasm actually using them from inferred types (TFA transformer does not push inferred constants into AST, it only attaches them as metadata). So it seems like the only use of inferred constants in dart2wasm is when signature shaker eliminates parameters having constant values and replaces those parameters with constants.
If that's a problem, I would suggest instead of clearing type attributes, we can add a flag (or pass a function predicate) to signature shaker to analyze the contents of the constant values which we use instead of parameters, and avoid eliminating parameters if their constant value references types from a different loading module or if constant value has a container larger than certain threshold.
Also, it seems like dart2wasm uses inferred closures for devirtualization, so that could be the reason for some part of the size reduction you're observing. In such a case, I would suggest to limit devirtualization (and inlining) of closures in dart2wasm if target closure is from another loading unit.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
by 1.1% (both compressed & uncompressed)Did you look at why there was a size reduction?
Note that attributes you're clearing also carry inferred closure, in addition to the inferred constant.
Artificially limiting inference for deferred libraries looks suboptimal: why can't we infer closures so their calls can be analyzed and infer constants such as null/true/false so their values can be used to specialize code, perform better devirtualization etc? As usual, (not) inferring something has a ripple and non-local effect which may affect inference results in many other places.
Note that weaker inference may result in the smaller size because of the reduced devirtualization and reduced inlining. But it doesn't mean we should prefer weaker inference. Instead, we can be more smart about using (or not using) the results of the inference.
For constants in particular, I don't see dart2wasm actually using them from inferred types (TFA transformer does not push inferred constants into AST, it only attaches them as metadata). So it seems like the only use of inferred constants in dart2wasm is when signature shaker eliminates parameters having constant values and replaces those parameters with constants.
If that's a problem, I would suggest instead of clearing type attributes, we can add a flag (or pass a function predicate) to signature shaker to analyze the contents of the constant values which we use instead of parameters, and avoid eliminating parameters if their constant value references types from a different loading module or if constant value has a container larger than certain threshold.
Also, it seems like dart2wasm uses inferred closures for devirtualization, so that could be the reason for some part of the size reduction you're observing. In such a case, I would suggest to limit devirtualization (and inlining) of closures in dart2wasm if target closure is from another loading unit.
The background here is: We have an app that we compile with deferred loading. I want to minimize the main module size. Amongst other things I compare what the main module pulls in dart2wasm vs dart2js. I've identified a few things dart2wasm puts in the main module that dart2js doesn't. One of them is certain constants. After finding out the user code where they come from, I can clearly see that user intended to access those constants in a deferred way in order not to pull it into the main unit. But because TFA propagates constants around and the signature shaking pass then pushes them into function bodies, which are included in the main unit they end up in the main module.
If that's a problem, I would suggest instead of clearing type attributes, we can add a flag (or pass a function predicate) to signature shaker to analyze the contents of the constant values which we use instead of parameters, and avoid eliminating parameters if their constant value references types from a different loading module or if constant value has a container larger than certain threshold.
It's not relevant if those constants reference types from a deferred component. The constants could be just data (lists, maps, numbers, strings). So that criteria isn't helpful I think.
Limiting a certain size is also strange: First we'd need to come up with some kind of size metric for a constant DAG. Then one may want to make this more precise and only measure the parts of the constant DAG that's not already in the main unit. Then let's assume we have a metric we're happy with, the constant DAGs are smaller than the threshold, so we propagate them, but we do that thousands of times, which still increases size, little by little.
In general: The propagation of constants is an optimization. A user shouldn't rely on this happening, not every backend may do it. So not doing it isn't a mistake per-se. Furthermore, if a user explicitly accesses things via a deferred import prefix, then they clearly want to avoid anything of this deferred import to be leaking (via optimizations) into the main module. Which is the reasoning I had when I made this CL.
In general this CL will have no effect on normal code - only very explicitly written user code - like `D.foo` - will prevent propagation of constants and I'd argue this is really what a user would expect here.
Yes, you're of course right that this could in some situations limit some optimizations, but I don't think this a bit issue.
Ideally (as mentioned in the commit message), we would actually propagate the constants around, but we would do so by attaching a deferred loading prefix with the constant we propagate. So whereever the constant flows and is used, we would also know which deferred loading prefix it was guarded by. Though this is a bigger undertaking and has it's own issues, so I opted for this solution.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
by 1.1% (both compressed & uncompressed)Martin KustermannDid you look at why there was a size reduction?
Note that attributes you're clearing also carry inferred closure, in addition to the inferred constant.
Artificially limiting inference for deferred libraries looks suboptimal: why can't we infer closures so their calls can be analyzed and infer constants such as null/true/false so their values can be used to specialize code, perform better devirtualization etc? As usual, (not) inferring something has a ripple and non-local effect which may affect inference results in many other places.
Note that weaker inference may result in the smaller size because of the reduced devirtualization and reduced inlining. But it doesn't mean we should prefer weaker inference. Instead, we can be more smart about using (or not using) the results of the inference.
For constants in particular, I don't see dart2wasm actually using them from inferred types (TFA transformer does not push inferred constants into AST, it only attaches them as metadata). So it seems like the only use of inferred constants in dart2wasm is when signature shaker eliminates parameters having constant values and replaces those parameters with constants.
If that's a problem, I would suggest instead of clearing type attributes, we can add a flag (or pass a function predicate) to signature shaker to analyze the contents of the constant values which we use instead of parameters, and avoid eliminating parameters if their constant value references types from a different loading module or if constant value has a container larger than certain threshold.
Also, it seems like dart2wasm uses inferred closures for devirtualization, so that could be the reason for some part of the size reduction you're observing. In such a case, I would suggest to limit devirtualization (and inlining) of closures in dart2wasm if target closure is from another loading unit.
The background here is: We have an app that we compile with deferred loading. I want to minimize the main module size. Amongst other things I compare what the main module pulls in dart2wasm vs dart2js. I've identified a few things dart2wasm puts in the main module that dart2js doesn't. One of them is certain constants. After finding out the user code where they come from, I can clearly see that user intended to access those constants in a deferred way in order not to pull it into the main unit. But because TFA propagates constants around and the signature shaking pass then pushes them into function bodies, which are included in the main unit they end up in the main module.
If that's a problem, I would suggest instead of clearing type attributes, we can add a flag (or pass a function predicate) to signature shaker to analyze the contents of the constant values which we use instead of parameters, and avoid eliminating parameters if their constant value references types from a different loading module or if constant value has a container larger than certain threshold.
It's not relevant if those constants reference types from a deferred component. The constants could be just data (lists, maps, numbers, strings). So that criteria isn't helpful I think.Limiting a certain size is also strange: First we'd need to come up with some kind of size metric for a constant DAG. Then one may want to make this more precise and only measure the parts of the constant DAG that's not already in the main unit. Then let's assume we have a metric we're happy with, the constant DAGs are smaller than the threshold, so we propagate them, but we do that thousands of times, which still increases size, little by little.
In general: The propagation of constants is an optimization. A user shouldn't rely on this happening, not every backend may do it. So not doing it isn't a mistake per-se. Furthermore, if a user explicitly accesses things via a deferred import prefix, then they clearly want to avoid anything of this deferred import to be leaking (via optimizations) into the main module. Which is the reasoning I had when I made this CL.
In general this CL will have no effect on normal code - only very explicitly written user code - like `D.foo` - will prevent propagation of constants and I'd argue this is really what a user would expect here.
Yes, you're of course right that this could in some situations limit some optimizations, but I don't think this a bit issue.
Ideally (as mentioned in the commit message), we would actually propagate the constants around, but we would do so by attaching a deferred loading prefix with the constant we propagate. So whereever the constant flows and is used, we would also know which deferred loading prefix it was guarded by. Though this is a bigger undertaking and has it's own issues, so I opted for this solution.
You're making a trade-off by suppressing unknown number of optimizations which affect both code size and performance. At the very least, I would expect this to be done under a flag which is turned off by default.
It's not relevant if those constants reference types from a deferred component. The constants could be just data (lists, maps, numbers, strings). So that criteria isn't helpful I think.
If the constant is a null, true, false, integer, double etc it is definitely not increasing size. What causes the size increase? Can you come up with a more specific criteria?
Limiting a certain size is also strange: First we'd need to come up with some kind of size metric for a constant DAG. [...]
It is easy to measure size/weight of a DAG - we just need to remember visited nodes to avoid visiting/counting multiple times. It is not very relevant that some parts of DAG may be present in the main unit - it is only a heuristic and we just need to make sure it covers the case we see in practice and does not affect the vast majority of other cases. It also makes sense to make a threshold value a TFA option so we can tune it if needed (with the default == unlimited).
Then let's assume we have a metric we're happy with, the constant DAGs are smaller than the threshold, so we propagate them, but we do that thousands of times, which still increases size, little by little.
Is it really the case in practice? Could you send me (in private) references to the relevant real examples?
Also, is there an option to change the source code to prevent this propagation instead of compiler (e.g. if there is a particular constant parameter we don't want to replace with a constant value, we can declare the method as entry point or add another call with a non-constant value of that parameter).
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
by 1.1% (both compressed & uncompressed)You're making a trade-off by suppressing unknown number of optimizations which affect both code size and performance.
In theory yes. In practice though it's very likely this will not have any noticeable negative affect on any existing apps. Using deferred imports is rare, especially for VM AOT. Those that do use it, will have very limited use of it. The limited use they do have (I suspect) will not benefit much from propagating constants across deferred prefix uses.
The other way is also true: By allowing constants to be propagated as they are now, we are making a trade-off by affecting code size of main module and thereby startup time.
The reason I opted for this is because of user expectation: A user that writes `D.foo` (where `D` is the deferred import prefix) most certainly doesn't want anything from the deferred library to be pulled into the main module - that's why they use a deferred import in the first place. (At least that was my reasoning)
At the very least, I would expect this to be done under a flag which is turned off by default.
I can add a flag. The downside would be that we may have less test coverage of code. Maybe the flag would be off in VM AOT but it would default to be on on dart2wasm.
If the constant is a null, true, false, integer, double etc it is definitely not increasing size.
Happy to special case null/false/true/int/double (possibly also small strings).
What causes the size increase? Can you come up with a more specific criteria?
It took me some hours today to debug the dart2wasm vs dart2js difference again (forgot where it was). I can forward g3 pointers offline.
The short answer is the `D.foo` is a `static final` with a tear off constant as value. That tear off hangs on to other things transitively that then all get pulled into the main unit.
There may be other situations (in the big g3 app) as well where this CL benefits - I can imagine composed constants of maps/lists/...
> Also, is there an option to change the source code to prevent this propagation instead of compiler (e.g. if there is a particular constant parameter we don't want to replace with a constant value, we can declare the method as entry point or add another call with a non-constant value of that parameter).
It's not about that specifically.
As mentioned earlier we can solve this problem also differently, in a way that may preserve the optimizations you care about, but has other downsides: We can make TFA not just propagate the constant but also the deferred load guard. Then wherever we use this constant, we'd emit a load guard check before using it. That will mean we do more load guard checks (which are somewhat fast, a lookup in a set or map):
https://dart-review.googlesource.com/c/sdk/+/478380
A somewhat similar approach would be to do the same as the link above, but instead of emitting a load guard, use some kernel annotation information, then the backends could read that to recover the information about the load guard.
I'll forward some g3 pointer offline.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
So the culprit is the propagation of tear-offs (TearOffConstant) from deferred libraries. Did you see any other examples, or we can assume that this is it?
Given two Member objects, how hard is it to tell if they belong to the same loading unit or not (in dart2wasm)? Is it possible to pass such predicate to TFA/signature shaker, so it could inspect the constant value of the parameter and check if the enclosing member and member referenced in the TearOffConstant belong to the same loading unit? If that's feasible, then we should just disable elimination of such parameters.
Otherwise, I think it is acceptable to clear constant attribute referencing tear-offs (but not other constants and not closure attribute), and only when receiving a value from a deferred call - for the value of the `Let` expression and not for all sub-expressions as you currently implemented.
Re: propagation of deferred load guards: I don't see advantages of that approach. The use of the constant is already dominated by a load guard (otherwise constant would not be propagated), it's just dart2wasm doesn't currently see it as it has limited knowledge.
Actually, your idea of inserting additional load guards makes me thinking if we could continue replacing parameters with constants in signature shaker, but handle those constants in dart2wasm. Signature shaker can mark the replacement `ConstantExpression` in a metadata. After that, when dart2wasm generates a `ConstantExpression` it can consult metadata to check if this is a propagated value of the parameter. In such a case, it can inspect the constant, and if it turns out to be a tear-off from another loading unit it can handle the constants as if it was from a deferred component (adding a load guard or just an indirection as needed). Maybe we don't even needed a metadata - dart2wasm could just check all constants if they contain tear-offs of deferred code and need to be a part of a deferred unit, and automatically place those constants to the corresponding deferred loading unit. This approach won't suppress any optimizations and looks solid to me. WDYT?
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
So the culprit is the propagation of tear-offs (TearOffConstant) from deferred libraries.
Well I started this work as soon as I hit one particular place where it clearly had a negative impact to propagate the constant out of deferred load guards.
The problem can happen with any constant - TearOffConstant happens to pull in code, other constants happen to pull in other constants, or large strings, etc. Because of that I don't think we should solve this specially for tear offs.
Did you see any other examples, or we can assume that this is it?
If I would dig deep enough it's very likely I can find cases where we load constants earlier than we need to due to TFA propagation of constants.
But I don't think it makes sense for me to spend a week digging through lots of code just to make a proof.
IMHO the problem is quite well understood and it should be solved in a general way.
Yes, I agree we can excempt inferred closures and bool/int/double/... constants.
Given two Member objects, how hard is it to tell if they belong to the same loading unit or not (in dart2wasm)? Is it possible to pass such predicate to TFA/signature shaker, so it could inspect the constant value of the parameter and check if the enclosing member and member referenced in the TearOffConstant belong to the same loading unit? If that's feasible ...
The (rather fine grained) partitioning of the app into modules happens after TFA. Doing it before TFA would lead to worse partitioning. So at TFA time we don't know how the code will be split up yet.
Otherwise, I think it is acceptable to clear constant attribute referencing tear-offs (but not other constants and not closure attribute)
This problem isn't limited to tearoffs, large string constants, large constant DAGs. It's a conceptual thing that I think warrants a general solution (i.e. not special case tear offs).
for the value of the Let expression and not for all sub-expressions as you currently implemented.
The desugaring isn't always exactly `let CheckLibraryIsLoaded in #Constant`. There are cases where use is burried deeper in the let body (bad AST encoding, yes, see e.g. https://github.com/dart-lang/sdk/issues/61764#issuecomment-3425917364)
> Actually, your idea of inserting additional load guards makes me thinking if we could continue replacing parameters with constants in signature shaker, but handle those constants in dart2wasm. Signature shaker can mark the replacement ConstantExpression in a metadata.
That sounds like what I wrote about in "A somewhat similar approach would be to do the same as the link above, but instead of emitting a load guard, use some kernel annotation information, then the backends could read that to recover the information about the load guard."
Yes, I could try that. I would make signature shaker put kernel metadata on the ConstantExpression and also add to existing inferred type metadata the deferred loading guard information.
Maybe we don't even needed a metadata - dart2wasm could just check all constants if they contain tear-offs of deferred code and need to be a part of a deferred unit, and automatically place those constants to the corresponding deferred loading unit. This approach won't suppress any optimizations and looks solid to me. WDYT?
Again, I view this as a conceptual issue, it's very clear to me that a user may write this
```
import 'bigdata.dart' as D;
Future loadAndPocessData() async {
await D.loadLibrary();
processIt(D.veryLargeJsonConstantField);
}
```A developer who writes this code would clearly have the expectation that `veryLargeJsonConstantField` is not bundled in the main module.
So the VM AOT has the same issue it would bundle `veryLargeJsonConstantField` very likely in the main module. The solution with kernel metadata which I could implement would solve my issue as I can change dart2wasm's deferred loading partitioning algorithm to take this metadata into account. BUT the VM's deferred loading will not be fixed by it, as it does coarse-grained library-based partitioning. (The current CL instead may handle this for the VM as well)
So if you strongly insist on this metadata approach, I could try that out.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
The use of the constant is already dominated by a load guard (otherwise constant would not be propagated), it's just dart2wasm doesn't currently see it as it has limited knowledge.
Yes, that's correct. The partitioning algorithm doesn't do dataflow analysis and therefore doesn't have information like this (e.g. all callsites of method X happen under deferred module guard D)
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Update from offline discussion: The two compromises:
compromise-1) special case `TearOffConstant`s only and only if `let _ = CheckLibraryIsLoaded() in <expr>` contains it at `<expr>` and not subexpressions
compromise-2) propagate a isDeferredUse boolean together with the constant: Then attach the boolean in kernel metadata in places we use the propgated constant. Relying on the assumption that the actual `<expr>` still has uses of it (e.g. `StaticGet` of a `static final x = <constant>`). The backend can then treat such isDeferredConstantUse constants as weak in the partitioning of the app into deferred units.
I've made a PoC of compromise-2) now, see https://dart-review.googlesource.com/c/sdk/+/478366 and it seems it also gives me 1% size reduction.
Though there's a number of reasons I'm not all too happy with this
- Kernel AST doesn't reliably provide thing that was accessed deferred (i.e. the `<expr>` in `let ... in <expr>` can be complex, see e.g. https://github.com/dart-lang/sdk/issues/61764#issuecomment-3425917364) - making this a hit and miss. Though CFE team is now working on this issue.
- It's very britle, filled with assumptions like no unreachable code eliminator running after TFA (e.g. which may remove unused `StaticGet`, may remove field and suddently assumptions break, and the deferred partitioning logic is incorrectly treating the constant reference as weak)
- It's lots of complexity for very little (if any) gain
So I'd find compromise-1) a little better, but that one leaves the issue of propagating large constant DAGs (e.g. json like data) across deferred boundaries.
I'll wait for Slava to return to get another opinion on this.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
final (:constant, :isDeferred) = replacement;We don't really need to know if the constant is "deferred". We just need to annotate all `ConstantExpression` nodes introduced by the signature shaker and treat them as weak when computing deferred loading units. This is correct as all these uses of constants are synthesized from constant propagation and not present in the original program.
This would significantly simplify this approach - we would not need to adjust TFA summaries, propagate "isDeferred" attribute etc.
Related details
- Martin Kustermann
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |