[S] Change in dart/sdk[main]: [dart2wasm] Use `JSStringImpl` class for string value types
Ömer Ağacan (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Martin Kustermann
Code-Owners
Code-Review
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")
translateType(InterfaceType(jsStringClass, Nullability.nonNullable))I think the original code was better. Let me explain why.
When Dart code has `String x` then we want to use `translateType(String)`. So if
- the implementation (like before) has multiple string classes, the `translateType(String)` may result in a base class (like `StringBase` before)
- the implementation (like now) has only one string class, then `translateType(String)` should result already in the wasm struct for `StringImpl` - as it's the only implementation of `String`.
So the code (as written before) will work - irrespective of whether we have one or multiple string implementation classes.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
translateType(InterfaceType(jsStringClass, Nullability.nonNullable))I think the original code was better. Let me explain why.
When Dart code has `String x` then we want to use `translateType(String)`. So if
- the implementation (like before) has multiple string classes, the `translateType(String)` may result in a base class (like `StringBase` before)
- the implementation (like now) has only one string class, then `translateType(String)` should result already in the wasm struct for `StringImpl` - as it's the only implementation of `String`.
So the code (as written before) will work - irrespective of whether we have one or multiple string implementation classes.
We already assume that we have one string type which is `JSStringImpl`, e.g. in https://github.com/dart-lang/sdk/blob/9a2bc9327f9aee7ee7d019fb956dbb4a679dc2aa/sdk/lib/_internal/wasm/lib/string_patch.dart#L213-L214.
Multiple string types were terrible for performance (caused polymorphism everywhere) and we were happy to get rid of them with JS strings. Do we have plans to bring back multiple string types?
Related details
- Martin Kustermann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
translateType(InterfaceType(jsStringClass, Nullability.nonNullable))Ömer AğacanI think the original code was better. Let me explain why.
When Dart code has `String x` then we want to use `translateType(String)`. So if
- the implementation (like before) has multiple string classes, the `translateType(String)` may result in a base class (like `StringBase` before)
- the implementation (like now) has only one string class, then `translateType(String)` should result already in the wasm struct for `StringImpl` - as it's the only implementation of `String`.
So the code (as written before) will work - irrespective of whether we have one or multiple string implementation classes.
We already assume that we have one string type which is `JSStringImpl`, e.g. in https://github.com/dart-lang/sdk/blob/9a2bc9327f9aee7ee7d019fb956dbb4a679dc2aa/sdk/lib/_internal/wasm/lib/string_patch.dart#L213-L214.
Multiple string types were terrible for performance (caused polymorphism everywhere) and we were happy to get rid of them with JS strings. Do we have plans to bring back multiple string types?
Related details
- Martin Kustermann
Code-Owners
Code-Review
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")
translateType(InterfaceType(jsStringClass, Nullability.nonNullable))Ömer AğacanI think the original code was better. Let me explain why.
When Dart code has `String x` then we want to use `translateType(String)`. So if
- the implementation (like before) has multiple string classes, the `translateType(String)` may result in a base class (like `StringBase` before)
- the implementation (like now) has only one string class, then `translateType(String)` should result already in the wasm struct for `StringImpl` - as it's the only implementation of `String`.
So the code (as written before) will work - irrespective of whether we have one or multiple string implementation classes.
We already assume that we have one string type which is `JSStringImpl`, e.g. in https://github.com/dart-lang/sdk/blob/9a2bc9327f9aee7ee7d019fb956dbb4a679dc2aa/sdk/lib/_internal/wasm/lib/string_patch.dart#L213-L214.
Multiple string types were terrible for performance (caused polymorphism everywhere) and we were happy to get rid of them with JS strings. Do we have plans to bring back multiple string types?
Right now we don't have plans on introducing multiple string types. Maybe in the future if we target pure wasm environments without a JS embedder we need custom strings and maybe then we need OneByte/TwoByte strings again - who knows.
But that doesn't matter for this discussion.
The question is whether we should bake in the specific string implementation in the entire compiler (and possibly runtime) vs using `String` and let the compiler figure out itself that there's only one implementation class and let it use that.
There doesn't seem any reason here to hard code this in.
The same is actually true for the other members above, see for example
```
late final w.RefType runtimeTypeType =
translateType(coreTypes.typeNonNullableRawType) as w.RefType;
```
We don't bake in `_Type` here either and instead use the `Type` and let the compiler figure out that all implementations of `Type` share `_Type` as base class and it should therefore use it's wasm struct.
Related details
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |