[XL] Change in dart/sdk[main]: [record_use] Default argument values
Daco Harkes (Gerrit)
Daco Harkes voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 3 comments![]( "Open in Gerrit")
bool hasNoElision(MemberEntity member) {We're modifying the runtime behavior of programs somewhat significantly with these record use annotations. As a user of the annotations I wouldn't want my code to get worse because I'm using them.
Suppose a package owner decides to add one of these annotations on a class/member. Some users of that package might not use the record use data but they still have to pay the penalty for it.
This applies to both the inlining and now this eliding logic. I know it makes the tracking a bit easier to do these but I wonder if we can't find a way to preserve the runtime optimizations.
if (_hasPragma(member, PragmaAnnotation.noElision) ||Similar to what we did with inlining can we use the `implies` map here?
final cls = member.enclosingClass;I don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Commit-Queue | +1 |
3 comments
We're modifying the runtime behavior of programs somewhat significantly with these record use annotations. As a user of the annotations I wouldn't want my code to get worse because I'm using them.
Suppose a package owner decides to add one of these annotations on a class/member. Some users of that package might not use the record use data but they still have to pay the penalty for it.
This applies to both the inlining and now this eliding logic. I know it makes the tracking a bit easier to do these but I wonder if we can't find a way to preserve the runtime optimizations.
Some users of that package might not use the record use data but they still have to pay the penalty for it.
The users of the package will never see the record use data. The package link hook itself will see it.
I know it makes the tracking a bit easier to do these but I wonder if we can't find a way to preserve the runtime optimizations.
For the TFA based ones which happen after constant folding we can. If we record before TFA and then filter the recordings. But I believe that is more complicated than the current approach. So, I'd rather postpone such approach until we have a use case in which we want both the recordings (binary size reduction of app) and the optimizations (speed).
Similar to what we did with inlining can we use the `implies` map here?
Done.
Unfortunately, we cannot do it not with the enclosing class.
I don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
I believe it should. Because we record const instances _and_ all constructor calls to give a complete picture of all instances.
Maybe we should consider having more precise control in the record use annotations: E.g. for static calls you might not be interested in some parameters, so it's fine to elide those. And of const instances you might not be interested in some fields and for constructor calls you might not be interested in some parameters. https://github.com/dart-lang/native/issues/3206 Lets discuss on that issue.
Related details
- Nate Biggs
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 5 comments![]( "Open in Gerrit")
final cls = member.enclosingClass;Daco HarkesI don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
I believe it should. Because we record const instances _and_ all constructor calls to give a complete picture of all instances.
Maybe we should consider having more precise control in the record use annotations: E.g. for static calls you might not be interested in some parameters, so it's fine to elide those. And of const instances you might not be interested in some fields and for constructor calls you might not be interested in some parameters. https://github.com/dart-lang/native/issues/3206 Lets discuss on that issue.
For my education - can you share a bit more (or link to a doc with details) why we need to avoid eliding the parameters? I understand that it is different than what the user wrote, but ideally they are not making decisions based on the parameters that weren't used.
Part of the implication is whether we could have a special marker to indicate an elided parameter, rather than recomputing the default value. This would still preserve the "shape" the developer expects, but have a special indicator that clarifies that the parameter was not relevant.
(classPragmaAnnotations[cls]?.contains(PragmaAnnotation.recordUse) ??A few thoughts. Not sure if this was already discussed with Nate.
(a) Now that you have the `implies` above - does that mean that noElision is also in the class? Could we check for that instead so we don't encode the implication in multiple places? (I'd still include a comment to explain this is motivated by the record-use scenario, though.)
(b) This is looking very much like the idea of hierarchical metadata that Stephen was pursuing in the past. See for example how late variables are annotated here and the use of DirectiveContet.parent. Would that be a cleaner approach?
final value = _findConstant(arguments[argumentIndex++]);mmm... this increment seems premature, the query on the next line should be based on the previous index, correct?
if (argumentIndex <nit: let's combine with the above to reduce duplicate logic. Suggestion, pushdown the condition to the definition of `value`, so the entire body of this closure may look like:
```
bool value = argumentIndex < arguments.length
? _findConstant(arguments[argumentIndex])
: defaultValue;
if (argumentIndex < ...positionalParameters) {
positionalArguments.add(argumentValue);
} else {
namedArguments[name!] = argumentValue;
}
argumentIndex++;
```
} else if (argumentIndex < arguments.length) {ditto - hopefully this can also be combined into a single block
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes voted and added 5 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Commit-Queue | +1 |
5 comments
final cls = member.enclosingClass;Daco HarkesI don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
Sigmund CheremI believe it should. Because we record const instances _and_ all constructor calls to give a complete picture of all instances.
Maybe we should consider having more precise control in the record use annotations: E.g. for static calls you might not be interested in some parameters, so it's fine to elide those. And of const instances you might not be interested in some fields and for constructor calls you might not be interested in some parameters. https://github.com/dart-lang/native/issues/3206 Lets discuss on that issue.
For my education - can you share a bit more (or link to a doc with details) why we need to avoid eliding the parameters? I understand that it is different than what the user wrote, but ideally they are not making decisions based on the parameters that weren't used.
Part of the implication is whether we could have a special marker to indicate an elided parameter, rather than recomputing the default value. This would still preserve the "shape" the developer expects, but have a special indicator that clarifies that the parameter was not relevant.
We are not always computing the default value. Parameter elision can also happen if only one value was ever passed to a function. That value then gets inlined in the body of that function. This value does not have to be the default value. And the consumer of the recorded uses needs to know what that value was.
(classPragmaAnnotations[cls]?.contains(PragmaAnnotation.recordUse) ??A few thoughts. Not sure if this was already discussed with Nate.
(a) Now that you have the `implies` above - does that mean that noElision is also in the class? Could we check for that instead so we don't encode the implication in multiple places? (I'd still include a comment to explain this is motivated by the record-use scenario, though.)
(b) This is looking very much like the idea of hierarchical metadata that Stephen was pursuing in the past. See for example how late variables are annotated here and the use of DirectiveContet.parent. Would that be a cleaner approach?
RE A: The `implies` only works for pragmas within a single definition. But for recording instances the record-use pragma is only on the class and it has effects to not remove any constructors parameters and also not remove any fields. We cannot express this in `implies`.
I've added a comment.
RE B: Oh interesting TIL! I don't think that would work here though. We only want to put the no-elision on the constructors when the record-use pragma is on the class, we don't want to put it on all methods too.
final value = _findConstant(arguments[argumentIndex++]);mmm... this increment seems premature, the query on the next line should be based on the previous index, correct?
Done
nit: let's combine with the above to reduce duplicate logic. Suggestion, pushdown the condition to the definition of `value`, so the entire body of this closure may look like:
```
bool value = argumentIndex < arguments.length
? _findConstant(arguments[argumentIndex])
: defaultValue;
if (argumentIndex < ...positionalParameters) {
positionalArguments.add(argumentValue);
} else {
namedArguments[name!] = argumentValue;
}
argumentIndex++;
```
Done
ditto - hopefully this can also be combined into a single block
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sigmund Cherem (Gerrit)
Sigmund Cherem added 1 comment![]( "Open in Gerrit")
final cls = member.enclosingClass;Daco HarkesI don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
Sigmund CheremI believe it should. Because we record const instances _and_ all constructor calls to give a complete picture of all instances.
Maybe we should consider having more precise control in the record use annotations: E.g. for static calls you might not be interested in some parameters, so it's fine to elide those. And of const instances you might not be interested in some fields and for constructor calls you might not be interested in some parameters. https://github.com/dart-lang/native/issues/3206 Lets discuss on that issue.
Daco HarkesFor my education - can you share a bit more (or link to a doc with details) why we need to avoid eliding the parameters? I understand that it is different than what the user wrote, but ideally they are not making decisions based on the parameters that weren't used.
Part of the implication is whether we could have a special marker to indicate an elided parameter, rather than recomputing the default value. This would still preserve the "shape" the developer expects, but have a special indicator that clarifies that the parameter was not relevant.
We are not always computing the default value. Parameter elision can also happen if only one value was ever passed to a function. That value then gets inlined in the body of that function. This value does not have to be the default value. And the consumer of the recorded uses needs to know what that value was.
Another high-level question, we can take this offline too.
Is record-use always enabled? or is there a compiler flag to decide whether we are recording the data? Having a flag makes some of these debates and nuanced conditions less controversial, since choices that affect performance/code-size only happy when users opt-in, and we can iterate on their performance implication without affecting all existing users.
Related details
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
final cls = member.enclosingClass;Daco HarkesI don't think putting `@RecordUse` on a class should mean the constructor doesn't have it's fields elided. I think they should have to put the annotation on the constructor itself.
Though see above comment.
Sigmund CheremI believe it should. Because we record const instances _and_ all constructor calls to give a complete picture of all instances.
Maybe we should consider having more precise control in the record use annotations: E.g. for static calls you might not be interested in some parameters, so it's fine to elide those. And of const instances you might not be interested in some fields and for constructor calls you might not be interested in some parameters. https://github.com/dart-lang/native/issues/3206 Lets discuss on that issue.
Daco HarkesFor my education - can you share a bit more (or link to a doc with details) why we need to avoid eliding the parameters? I understand that it is different than what the user wrote, but ideally they are not making decisions based on the parameters that weren't used.
Part of the implication is whether we could have a special marker to indicate an elided parameter, rather than recomputing the default value. This would still preserve the "shape" the developer expects, but have a special indicator that clarifies that the parameter was not relevant.
Sigmund CheremWe are not always computing the default value. Parameter elision can also happen if only one value was ever passed to a function. That value then gets inlined in the body of that function. This value does not have to be the default value. And the consumer of the recorded uses needs to know what that value was.
Another high-level question, we can take this offline too.
Is record-use always enabled? or is there a compiler flag to decide whether we are recording the data? Having a flag makes some of these debates and nuanced conditions less controversial, since choices that affect performance/code-size only happy when users opt-in, and we can iterate on their performance implication without affecting all existing users.
It is behind a flag right now. We will enable the flag in AOT by default for link hooks. (It's not enable-able at all for JIT, we don't record anything in JIT.)
I think my high level view here is that if we get a ton of adoption, everyone relies on it to tree-shake their assets, and then some Dart code is slow, we have a business case for optimizing performance. Right now, we have a business case for correctness only.
Related details
- Sigmund Cherem
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |