[M] Change in dart/sdk[main]: [record_use] Symbol constants
Daco Harkes (Gerrit)
New activity on the change![]( "Open in Gerrit")
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")
final String? symbolLibraryUri;I don't love adding a field to every `ConstructedConstantValue` as this can have a significant impact on memory usage. When compiling large programs we may have a lot of these objects in the heap at any given time so small changes like this can compound.
I wonder if we can keep a separate more compact Map on the side mapping symbol constants to their libraries. The `JsKernelToElementMap` contains the `ConstantValuefier` that generates these objects. In there is a `cache` which we can iterate over and save just the `SymbolConstant`s and their corresponding dart2js `ConstantValue`s.
This map should be easy enough to serialize and deserialize as part of the ElementMap.
String? libraryUri;Usually we store library URIs as `Uri` objects, not Strings.
if (libraryUri != null && !libraryUri.startsWith('package:')) {Use `Uri.isScheme`
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
I don't love adding a field to every `ConstructedConstantValue` as this can have a significant impact on memory usage. When compiling large programs we may have a lot of these objects in the heap at any given time so small changes like this can compound.
I wonder if we can keep a separate more compact Map on the side mapping symbol constants to their libraries. The `JsKernelToElementMap` contains the `ConstantValuefier` that generates these objects. In there is a `cache` which we can iterate over and save just the `SymbolConstant`s and their corresponding dart2js `ConstantValue`s.
This map should be easy enough to serialize and deserialize as part of the ElementMap.
Done
Usually we store library URIs as `Uri` objects, not Strings.
Done
if (libraryUri != null && !libraryUri.startsWith('package:')) {Daco HarkesUse `Uri.isScheme`
Done
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 voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sigmund Cherem
| Code-Review | +1 |
5 comments
Uri? libraryUri,undo: now that we no longer store it in a field, we can drop this parameter here.
libraryUri,nits:
* now that we are dropping this parameter, consider a small simplification moving the value declaration again to the top of the method.
* use `?.`
For instance,
```
final value = constant_system.createSymbol(elementMap.commonElements, node.name);
final libraryUri = node.libraryReference?.asLibrary.importUri;
if (libraryUri != null) {
elementMap.registerSymbolLibrary(value, libraryUri);
}
return value;
```
or maybe combine the conditionals:
```
final value = constant_system.createSymbol(elementMap.commonElements, node.name);
if (node.libraryReference != null) {
elementMap.registerSymbolLibrary(
value,
node.libraryReference!.asLibrary.importUri,
);
}
return value;
```
abstract class JsToElementMap implements IrToElementMap {I think we can undo all changes in this file, instead depend directly on `JsToElementMap` in `RecordUseValueConverter`, which internally is already depending on J abstractions like the JCommonElements.
if (libraryUri != null && !libraryUri.isScheme('package')) {consider documenting why we are excluding other schemes?
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
undo: now that we no longer store it in a field, we can drop this parameter here.
Done
nits:
* now that we are dropping this parameter, consider a small simplification moving the value declaration again to the top of the method.
* use `?.`
For instance,```
final value = constant_system.createSymbol(elementMap.commonElements, node.name);
final libraryUri = node.libraryReference?.asLibrary.importUri;
if (libraryUri != null) {
elementMap.registerSymbolLibrary(value, libraryUri);
}
return value;
```or maybe combine the conditionals:
```
final value = constant_system.createSymbol(elementMap.commonElements, node.name);
if (node.libraryReference != null) {
elementMap.registerSymbolLibrary(
value,
node.libraryReference!.asLibrary.importUri,
);
}
return value;
```
Done
abstract class JsToElementMap implements IrToElementMap {I think we can undo all changes in this file, instead depend directly on `JsToElementMap` in `RecordUseValueConverter`, which internally is already depending on J abstractions like the JCommonElements.
Done
if (libraryUri != null && !libraryUri.isScheme('package')) {consider documenting why we are excluding other schemes?
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sigmund Cherem
| Code-Review | +1 |
3 comments
this._irToElementMap,It may be worth iterating a bit on the parameters here. It seems that the uses below are that mainly the jsMap is used to get the parameters we used to have before, and the irMap is used in a way where the jsMap could be used instead?
Suggestions:
- move from using irMap to jsMap to call `getSymbolLibraryUri`. That will allow us to remove one kind of dependency here.
- keep the original fields for elementenvironment/commonelements and use constructor initializers instead?
JElementEnvironment get _elementEnvironment =>If we decide to keep the getters, consider copying the comment you use to have on the field to this declaratinon.
// 'package:' library. This is because symbols from other schemes (likeQ: Will we ever have assets from `dart:` libraries?
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")
// 'package:' library. This is because symbols from other schemes (likeQ: Will we ever have assets from `dart:` libraries?
Right now I'm not intending this to be used inside the Dart or Flutter built-in libraries. That being said, if we ever have a use case for it, I don't see why we couldn't add support for that.
Related details
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 2 comments![]( "Open in Gerrit")
Votes added by Daco Harkes
| Commit-Queue | +1 |
2 comments
It may be worth iterating a bit on the parameters here. It seems that the uses below are that mainly the jsMap is used to get the parameters we used to have before, and the irMap is used in a way where the jsMap could be used instead?
Suggestions:
- move from using irMap to jsMap to call `getSymbolLibraryUri`. That will allow us to remove one kind of dependency here.
- keep the original fields for elementenvironment/commonelements and use constructor initializers instead?
Done
If we decide to keep the getters, consider copying the comment you use to have on the field to this declaratinon.
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sigmund Cherem
| Code-Review | +1 |
- 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 Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[record_use] Symbol constants
Closes: https://github.com/dart-lang/native/issues/3053
TEST=pkg/compiler/test/record_use/record_use_test.dart
TEST=pkg/dart2wasm/test/record_use_test.dart
TEST=pkg/vm/test/transformations/record_use_test.dart
- M pkg/compiler/lib/src/constants/values.dart
- M pkg/compiler/lib/src/ir/element_map.dart
- M pkg/compiler/lib/src/ir/visitors.dart
- M pkg/compiler/lib/src/js_emitter/record_use_emitter.dart
- M pkg/compiler/lib/src/js_model/element_map.dart
- M pkg/compiler/lib/src/js_model/element_map_impl.dart
- M pkg/compiler/lib/src/kernel/element_map_impl.dart
- M pkg/compiler/lib/src/universe/recorded_use.dart
- M pkg/vm/lib/transformations/record_use/record_use.dart
- A pkg/vm/testcases/transformations/record_use/lib/symbol_const.dart
- A pkg/vm/testcases/transformations/record_use/lib/symbol_const.dart.aot.expect
- A pkg/vm/testcases/transformations/record_use/lib/symbol_const.dart.json.expect
Code-Review: +1 by Sigmund Cherem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |