[M] Change in dart/sdk[main]: [vm/sdk] Use FinalThreadLocal for Iterable toString.
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
1 comment
if (!_toStringVisiting.isBound) {Alexander AprelevGiven that we introduced `FinalThreadLocal` anyway... Maybe you can rewrite this code to use that instead. And I think when you do that you can share the iterableToFullString and iterableToShortString methods back - it becomes platform independent.
Platform dependent part is `toStringVisiting`, which for VM becomes:
```
List get toStringVisiting => _toStringVisiting.value;
...
```And the rest of code can be shared.
Done
Related details
- Slava Egorov
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Slava Egorov
| Code-Review | +1 |
1 comment
for (int i = 0; i < toStringVisiting.length; i++) {Consider caching result of getter, e.g.
```
final visiting = toStringVisiting;
for (int i = 0; i < visiting.length; i++) {
if (identical(object, visiting[i])) return true;
}
return false;
```
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
2 comments
Consider caching result of getter, e.g.
```
final visiting = toStringVisiting;
for (int i = 0; i < visiting.length; i++) {
if (identical(object, visiting[i])) return true;
}
return false;
```
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 voted and added 8 comments![]( "Open in Gerrit")
Votes added by Lasse Nielsen
| Code-Review | -1 |
8 comments
Don't move map-function into `Iterable` and don't make the helper functions public.
const smallChangeThreshold = 13;(Arbitrary number just changed arbitrarily, for _reasons_! 😜)
static List<Object> get toStringVisiting => _toStringVisiting;That looks like it's just
```dart
@patch
static final List<Object> toStringVisiting = [];
```
with more steps. Unless that's not an allowed patch, then it's *necessary* more steps. Is it necessary?
static String mapToString(Map<Object?, Object?> m) => Iterable.mapToString(m);Don't move this function. It doesn't belong in `Iterable`.
static String mapToString(Map<Object?, Object?> m) {This is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.
If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?
If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
static bool isToStringVisiting(Object object) {This should *probably* not be public.
external static List<Object> get toStringVisiting;This should _definitely_ not be public.
(Which tips the `toStringVisiting` towards not being public either.)
Related details
- Alexander Aprelev
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 3 comments![]( "Open in Gerrit")
static List<Object> get toStringVisiting => _toStringVisiting;That looks like it's just
```dart
@patch
static final List<Object> toStringVisiting = [];
```
with more steps. Unless that's not an allowed patch, then it's *necessary* more steps. Is it necessary?
Patching getter with final variable doesn't work, and I believe it has to remain getter for vm/iterable_patch.dart FinalThreadLocal implementation.
static String mapToString(Map<Object?, Object?> m) => Iterable.mapToString(m);Don't move this function. It doesn't belong in `Iterable`.
The function still remains, it's just that its implementation delegates to Iterable.mapToString because that's where patched toStringVisiting lives.
Alexander AprelevWhy moved?
They were moved because toStringVisiting has to be patched, has different implementation on vm vs other platforms.
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. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
3 comments
This is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
I added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
This should *probably* not be public.
Done
This should _definitely_ not be public.
(Which tips the `toStringVisiting` towards not being public either.)
- Lasse Nielsen
- Slava Egorov
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 1 comment![]( "Open in Gerrit")
static String mapToString(Map<Object?, Object?> m) {Alexander AprelevThis is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
I added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
Why does it need to be part of this class? Can all the "toStringVisiting" behavior, and it's static state, be moved to a shared library? The public APIs can be small forwarding implementations. Changing public API for implementation detail changes worries me.
Related details
- Alexander Aprelev
- Lasse Nielsen
- Slava Egorov
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
static String mapToString(Map<Object?, Object?> m) {Alexander AprelevThis is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
Nate BoschI added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
Why does it need to be part of this class? Can all the "toStringVisiting" behavior, and it's static state, be moved to a shared library? The public APIs can be small forwarding implementations. Changing public API for implementation detail changes worries me.
Why does it need to be part of this class?
For patching to work if I understand correctly patched methods needs to be part of some class. So previous public top-level-implementation(in internal.dart) won't work. This class already hosts recursive-friendly infrastructure for iterable toString operations, so it seems to be reasonable place to host map-specific toString operation as well.
Related details
- Lasse Nielsen
- Nate Bosch
- Slava Egorov
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 1 comment![]( "Open in Gerrit")
static String mapToString(Map<Object?, Object?> m) {Alexander AprelevThis is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
Nate BoschI added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
Alexander AprelevWhy does it need to be part of this class? Can all the "toStringVisiting" behavior, and it's static state, be moved to a shared library? The public APIs can be small forwarding implementations. Changing public API for implementation detail changes worries me.
Why does it need to be part of this class?
For patching to work if I understand correctly patched methods needs to be part of some class. So previous public top-level-implementation(in internal.dart) won't work. This class already hosts recursive-friendly infrastructure for iterable toString operations, so it seems to be reasonable place to host map-specific toString operation as well.
Ah I didn't realize it was related to limitations in patching, I doubt I'll be able to find any alternatives to suggest in that case.
Related details
- Alexander Aprelev
- Lasse Nielsen
- Slava Egorov
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Lasse Nielsen
| Code-Review | -1 |
2 comments
static String mapToString(Map<Object?, Object?> m) {Alexander AprelevThis is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
Nate BoschI added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
Alexander AprelevWhy does it need to be part of this class? Can all the "toStringVisiting" behavior, and it's static state, be moved to a shared library? The public APIs can be small forwarding implementations. Changing public API for implementation detail changes worries me.
Nate BoschWhy does it need to be part of this class?
For patching to work if I understand correctly patched methods needs to be part of some class. So previous public top-level-implementation(in internal.dart) won't work. This class already hosts recursive-friendly infrastructure for iterable toString operations, so it seems to be reasonable place to host map-specific toString operation as well.
Ah I didn't realize it was related to limitations in patching, I doubt I'll be able to find any alternatives to suggest in that case.
I'd really prefer to just not have this method on `Iterable`. It doesn't belong there, maps are not iterables.
Keep the function in `MapBase`, or move the implementation to `dart:internal` if you want to share it.
If that's a problem with `_istoStringVisiting` and `_toStringVisiting`, move them all to `dart:_internal`. The two `*toStringVisiting` declarations can be public there.
_(We really should have the core implementation in `dart:_core` which `dart:core` then exports *some* of. That would allow us to have public helpers in the same library, without exposing them to the world.)_
static String mapToString(Map<Object?, Object?> m) {Still think this shouldn't be in `Iterable`. Maps are not iterables, there is no reason for it to be *here*.
Keep it in `MapBase`, and move `*toStringVisiting` to `dart:internal`
so it can be used from both `dart:core` and `dart:collection`.
Related details
- Alexander Aprelev
- Slava Egorov
Code-Owners
Code-Review
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
3 comments
thank you Lasse for the suggestions.
static String mapToString(Map<Object?, Object?> m) {Alexander AprelevThis is a new public function.
It used to be in `MapBase` and now it's in `Iterable`.If it's no longer in `MapBase`, that's a breaking change, so don't remove it.
Also, it doesn't really belong in `Iterable`, maps are not `Iterable`, so why move it here?If it stays, it should have a `@Since("3.11")`, or whatever version it goes into, and be documented in the CHANGELOG.md.
If you want to share it without needing to import `dart:collection`, then put it in `dart:_internal` and let `MapBase` reference it there.
Nate BoschI added Since and added entry to changelog.
I'm not sure I understand how to share it since it needs to be part of this class.
Alexander AprelevWhy does it need to be part of this class? Can all the "toStringVisiting" behavior, and it's static state, be moved to a shared library? The public APIs can be small forwarding implementations. Changing public API for implementation detail changes worries me.
Nate BoschWhy does it need to be part of this class?
For patching to work if I understand correctly patched methods needs to be part of some class. So previous public top-level-implementation(in internal.dart) won't work. This class already hosts recursive-friendly infrastructure for iterable toString operations, so it seems to be reasonable place to host map-specific toString operation as well.
Lasse NielsenAh I didn't realize it was related to limitations in patching, I doubt I'll be able to find any alternatives to suggest in that case.
I'd really prefer to just not have this method on `Iterable`. It doesn't belong there, maps are not iterables.
Keep the function in `MapBase`, or move the implementation to `dart:internal` if you want to share it.
If that's a problem with `_istoStringVisiting` and `_toStringVisiting`, move them all to `dart:_internal`. The two `*toStringVisiting` declarations can be public there.
_(We really should have the core implementation in `dart:_core` which `dart:core` then exports *some* of. That would allow us to have public helpers in the same library, without exposing them to the world.)_
Done
Still think this shouldn't be in `Iterable`. Maps are not iterables, there is no reason for it to be *here*.
Keep it in `MapBase`, and move `*toStringVisiting` to `dart:internal`
so it can be used from both `dart:core` and `dart:collection`.
- Lasse Nielsen
- Nate Bosch
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lasse Nielsen
| Code-Review | +1 |
1 comment
- Alexander Aprelev
- Nate Bosch
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
friendly ping
Alexander Aprelev (Gerrit)
Alexander Aprelev voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Aprelev
| Commit-Queue | +1 |
1 comment
Adding Nate for updates to dart2wasm test expectations
Related details
- Nate Biggs
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Bosch (Gerrit)
Nate Bosch voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Bosch
| Code-Review | +1 |
1 comment
///[nit] Can we delete this? I see it's an existing pattern in this file, but I don't think it's a good pattern to keep.
Related details
- Alexander Aprelev
- Nate Biggs
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 2 comments![]( "Open in Gerrit")
still looking for +1 from dart2wasm test owners.
[nit] Can we delete this? I see it's an existing pattern in this file, but I don't think it's a good pattern to keep.
Thanks Nate, I put them there to group top-levels variables with the top-level test methods they used in. Without any delimiters it felt it was hard to see the reason for top-level variables to be interleaved with the metods. Do you have a better suggestion?
Related details
- Nate Biggs
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted Code-Review+1![]( "Open in Gerrit")
- Alexander Aprelev
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev voted Commit-Queue+2![]( "Open in Gerrit")
- Slava Egorov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Aprelev (Gerrit)
Alexander Aprelev added 1 comment![]( "Open in Gerrit")
thank you Nate!
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[vm/sdk] Use FinalThreadLocal for Iterable toString.
This allows use of toString in isolategroup-bound callbacks.
BUG=https://github.com/dart-lang/sdk/issues/61030.
TEST=ci
CoreLibraryReviewExempt: effectively vm-only change
Change-Id: Iccdc218716cbbe2346aa85bb6134f4129debe057
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/451841
Reviewed-by: Nate Biggs <nate...@google.com>
Commit-Queue: Alexander Aprelev <a...@google.com>
Reviewed-by: Lasse Nielsen <l...@google.com>
Reviewed-by: Nate Bosch <nbo...@google.com>
- M pkg/dart2wasm/test/ir_tests/dyn_closure.wat
- M pkg/dart2wasm/test/ir_tests/dyn_closure_function_apply.wat
- M pkg/dart2wasm/test/ir_tests/dyn_closure_function_apply_named.wat
- M pkg/vm_snapshot_analysis/test/instruction_sizes_test.dart
- M sdk/lib/_internal/js_dev_runtime/patch/internal_patch.dart
- M sdk/lib/_internal/js_runtime/lib/internal_patch.dart
- M sdk/lib/_internal/vm/lib/internal_patch.dart
- M sdk/lib/_internal/wasm/lib/internal_patch.dart
- M sdk/lib/collection/collection.dart
- M sdk/lib/core/core.dart
- M sdk/lib/internal/internal.dart
- M tests/ffi/run_isolate_group_run_test.dart
Code-Review: +1 by Lasse Nielsen, +1 by Nate Biggs, +1 by Nate Bosch
Core-Library-Review: Code-Review+1 by Lasse Nielsen, Code-Review+1 by Nate Biggs, Code-Review+1 by Nate Bosch
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Bosch (Gerrit)
Nate Bosch added 1 comment![]( "Open in Gerrit")
Alexander Aprelev[nit] Can we delete this? I see it's an existing pattern in this file, but I don't think it's a good pattern to keep.
Thanks Nate, I put them there to group top-levels variables with the top-level test methods they used in. Without any delimiters it felt it was hard to see the reason for top-level variables to be interleaved with the metods. Do you have a better suggestion?
I think either `//` or blocking by only including blank lines between blocks are both slightly preferable to `///` which stands out as a blank doc to me, but the ROI on changing is not as high in that case so I'd also be fine leaving well enough alone.
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Core-Library-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |