[L] Change in dart/sdk[main]: Make concrete `_ZoneFunction`s not generic.
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
Another attempt, taking _smaller_ steps, towards making Zone monomorphic.
Just refactoring and getting rid of an unnecessary split between interface and implementation of a class that is now final anyway.
@dacoh...@google.com - You seem to be one of only two people owning dart2wasm tests.
Related details
- Daco Harkes
- Nate Bosch
Code-Owners
Code-Review
Core-Library-Review
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")
Another attempt, taking _smaller_ steps, towards making Zone monomorphic.
Just refactoring and getting rid of an unnecessary split between interface and implementation of a class that is now final anyway.
@dacoh...@google.com - You seem to be one of only two people owning dart2wasm tests.
@kuste...@google.com or the others in the WASM team are the real owners. I have write access to be able to update record_use things.
I have no idea why Gerrit doesn't suggest all people in the WASM_OWNERS file.
Related details
- Lasse Nielsen
- Martin Kustermann
- Nate Bosch
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen added 6 comments![]( "Open in Gerrit")
_ZoneVariables get _zoneVariables;Makes zone variables be tracked the same way as other zone-overrides.
They're the only non-function, but otherwise they're scoped the same way.
_ZoneRun _run;Introduces specialized subclasses for each type of zone values, so that the instance doesn't have to store type arguments. May reduce memory load a little.
There is no code that treats a zone value in general, they don't even need a common superclass, that's just to reuse code.
if (specification != null) {Allows `specification` to be null instead of passing in an empty specification object, so if you don't give a specification when forking, it can skip all the checks.
dynamic _recursiveLookup(_ZoneVariables variables, Object? key) {Split into two functions in hope that the lookup might be inlined.
(But that probably won't happen until `Zone` becomes one class, not `_CustomZone` and `_RootZone` with different `[]` operations.
throw "unreachable"; // TODO(lrn): Remove when type promotion works.It works. But now so does passing `null` instead of creating a dummy value.
final class const ZoneSpecification({(Maybe it's still too soon to use primary constructor syntax, https://github.com/dart-lang/sdk/issues/63589, but I don't actually know where to see that. I can convert back if necessary. Or have the AI do it?)
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
Daco HarkesAnother attempt, taking _smaller_ steps, towards making Zone monomorphic.
Just refactoring and getting rid of an unnecessary split between interface and implementation of a class that is now final anyway.
@dacoh...@google.com - You seem to be one of only two people owning dart2wasm tests.
@kuste...@google.com or the others in the WASM team are the real owners. I have write access to be able to update record_use things.
I have no idea why Gerrit doesn't suggest all people in the WASM_OWNERS file.
- Martin Kustermann
- Nate Bosch
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 7 comments![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
7 comments
(global $"WasmArray<WasmI32>[800]" (ref $Array<WasmI32>) <...>)(That's due to increase in number of classes)
// all fields.nit: remove this comment?
_ZoneRun _run;Introduces specialized subclasses for each type of zone values, so that the instance doesn't have to store type arguments. May reduce memory load a little.
There is no code that treats a zone value in general, they don't even need a common superclass, that's just to reuse code.
so that the instance doesn't have to store type arguments
Without digging, I would suspect that at least VM AOT and wasm store the `_ZoneValue.T` type argument in an extra field. i.e. I suspect we don't optimize this away.
If you pay the price for extra classes already, why not remove the common super class?
What would be the downside of making those records? i.e.
```
typedef _ZoneRun = (Zone, RunHandler);
...
```
?
_ZoneHandleUncaughtError _handleUncaughtError;Is there a reason why they can't be final fields?
dynamic _recursiveLookup(_ZoneVariables variables, Object? key) {Split into two functions in hope that the lookup might be inlined.
(But that probably won't happen until `Zone` becomes one class, not `_CustomZone` and `_RootZone` with different `[]` operations.
dbc: Having only one class would be great (In fact, at some point I thought I try myself, because I stumbled upon some bad code because of this)
var handler = implementation.value as RunHandler;This `_run` field is defined via
```
_ZoneRun _run;
```
and we have
```
final class const _ZoneRun(super.zone, super.value)
extends _ZoneValue<RunHandler>;
sealed class _ZoneValue<T extends Object> {
final _Zone zone;
final T value;
const _ZoneValue(this.zone, this.value);
}
```
So shouldn't `implementation.value` have the type `RunHandler` type already?
Why this cast here?
valueMap = HashMap<Object?, Object?>.of(zoneValues);In general we should always use the default `Map` (which is used also for `{}` and `{}` const literals). This avoid pulling in 2 different map implementations into apps, reducing size.
Do you use `HashMap` intentionally here because we have prove it makes things meaningfully faster? If not, consider using `Map`
Related details
- Lasse Nielsen
- Nate Bosch
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Bosch (Gerrit)
Nate Bosch added 3 comments![]( "Open in Gerrit")
Can you expand the commit message with more details on the code change and the motivation for the changes?
"dartdoc_rev": "d0fac90eb4213db3d5f8d8ff360a4f4bcb4b7ae2",Is this intentionally included in this CL? It seems unrelated, but if it is related we should say how in the commit message.
/// This is always a [HashMap].[nit]
```suggestion
/// The `value` is always a [HashMap].
```
Related details
- Lasse Nielsen
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |