[M] Change in dart/sdk[main]: [vm/corelib] Optimize building of Maps in JSON decoder.
Slava Egorov (Gerrit)
Slava Egorov added 1 comment![]( "Open in Gerrit")
/// Token type for sharing private functionality between core libraries.Currently the only way to share private functionality between two different core libraries (e.g. dart:collection and dart:convert) without exposing it to the users is to move implementation into a private core library and then re-export pieces of it through core libraries.
Doing this with something like `dart:collection` to expose an efficient private method for creating a `Map` from the list key-value pairs is a big refactoring because `dart:collection` has a huge API surface.
So instead I came up with this _hack_. An alternative I considered is doing something like
```
// dart:_internal
// On VM this would redirect `method` to `_privateMethod` which normally can't be
// invoked.
@pragma('vm:redirect', ['dart:collection', '_privateMethod']);
external void method();
```
@l...@google.com Lasse, do you have any opinions on this?
Related details
- Lasse Nielsen
- Martin Kustermann
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 added 4 comments![]( "Open in Gerrit")
coreLibrariesPrivateFunctionality;Why not just import the function from `dart:_internal` directly.
Is it because the patch uses
void _populateUnsafe(List<Object?> kvPairs) {(Aboid abbreviations, `keyValuePairs` isn't that long.)
extension CollectionInternals on InternalFunctionalityToken {This declaration is publicly visible.
It may not be possible to create an object implementing `InternalFunctionalityToken`, but the extension exists in the public namespace and shows that `InternalFunctionalityToken` exists.
It's possible to name complete `CollectionInternals`, and write
`CollectionInternals(throw "banana").createMapFromKeyValuyeListUnsafe(...)`.
It should be hidden better.
If the functionality only needs to be used from inside `dart:` libraries, then just put it into `dart:_internal`, which they can all import, instead of putting it in `dart:collection`.
If it's used by non-`dart:` libraries ... why? And who? And like, don't. 😊
/// Token type for sharing private functionality between core libraries.Currently the only way to share private functionality between two different core libraries (e.g. dart:collection and dart:convert) without exposing it to the users is to move implementation into a private core library and then re-export pieces of it through core libraries.
Doing this with something like `dart:collection` to expose an efficient private method for creating a `Map` from the list key-value pairs is a big refactoring because `dart:collection` has a huge API surface.
So instead I came up with this _hack_. An alternative I considered is doing something like
```
// dart:_internal// On VM this would redirect `method` to `_privateMethod` which normally can't be
// invoked.
@pragma('vm:redirect', ['dart:collection', '_privateMethod']);
external void method();
```@l...@google.com Lasse, do you have any opinions on this?
Yep. Don't like visible hacks. (No matter how clever, the goal is to not expose any public names to users.)
I like the idea of allowing platform libraries to redirect to each other.
(I've been very tempted to just combine all the core libraries into on `dart:_impl` and export the individual libraries from there. It's not like they're not strongly connected anyway.)
I'll think about it and see if I can find an invisible alternative.
(Maybe we can have some small shared stub in `dart:_internal` that classes can opt in to being compatible with in some way. I hope, but no concrete idea yet.)
Related details
- Martin Kustermann
- Slava Egorov
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. |
Slava Egorov (Gerrit)
Slava Egorov added 4 comments![]( "Open in Gerrit")
Why not just import the function from `dart:_internal` directly.
Is it because the patch uses
That's because the function actually resides in `dart:collection` (which contains both public classes and their private implementations).
I have tried to move private implementations into `dart:_internal` but that does not work - because we have declared public interfaces `final`. :)
Anyway, new patch does VM specific redirection instead.
(Aboid abbreviations, `keyValuePairs` isn't that long.)
Done
extension CollectionInternals on InternalFunctionalityToken {This declaration is publicly visible.
It may not be possible to create an object implementing `InternalFunctionalityToken`, but the extension exists in the public namespace and shows that `InternalFunctionalityToken` exists.It's possible to name complete `CollectionInternals`, and write
`CollectionInternals(throw "banana").createMapFromKeyValuyeListUnsafe(...)`.It should be hidden better.
If the functionality only needs to be used from inside `dart:` libraries, then just put it into `dart:_internal`, which they can all import, instead of putting it in `dart:collection`.
If it's used by non-`dart:` libraries ... why? And who? And like, don't. 😊
You can write that but it will not execute :)
Anyway, I agree it is too ugly. Implemented `@pragma('vm:redirect', ...)` instead.
/// Token type for sharing private functionality between core libraries.Lasse NielsenCurrently the only way to share private functionality between two different core libraries (e.g. dart:collection and dart:convert) without exposing it to the users is to move implementation into a private core library and then re-export pieces of it through core libraries.
Doing this with something like `dart:collection` to expose an efficient private method for creating a `Map` from the list key-value pairs is a big refactoring because `dart:collection` has a huge API surface.
So instead I came up with this _hack_. An alternative I considered is doing something like
```
// dart:_internal// On VM this would redirect `method` to `_privateMethod` which normally can't be
// invoked.
@pragma('vm:redirect', ['dart:collection', '_privateMethod']);
external void method();
```@l...@google.com Lasse, do you have any opinions on this?
Yep. Don't like visible hacks. (No matter how clever, the goal is to not expose any public names to users.)
I like the idea of allowing platform libraries to redirect to each other.
(I've been very tempted to just combine all the core libraries into on `dart:_impl` and export the individual libraries from there. It's not like they're not strongly connected anyway.)I'll think about it and see if I can find an invisible alternative.
(Maybe we can have some small shared stub in `dart:_internal` that classes can opt in to being compatible with in some way. I hope, but no concrete idea yet.)
- Lasse Nielsen
- Martin Kustermann
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen voted and added 19 comments![]( "Open in Gerrit")
Votes added by Lasse Nielsen
| Code-Review | +1 |
19 comments
LGTM, with only small nits.
Lots of ideas for different ways to do the same thing, but if this is an improvement, then ship it. We can always improve more.
inCoreLibrary = node.importUri.scheme == 'dart';Use `.isScheme('dart')` instead of `.scheme == 'dart`. (It avoids allocation for `_SimpleUri`s. And is case insensitive.)
```dart
inCoreLibrary = node.importUri.isScheme('dart');
```
// Resolve redirection supplied via vm:redirect pragma if applicable.Maybe worth mentioning that it always stores a value in the `_redirectionCache`. If there is no redirection, it stores the target itself, so it's cheaper to look up a second time.
(Or insert a comment line in the code saying that. I had to read the code twice to notice that detail, which I think is fairly important.)
final [libraryUri, functionName] = targetName.split('#');This pattern match throws if the option string has more or less than one `#`. I'm not sure throwing is the best way to report an error.
Obviously that should never happen because we only end here for declarations inside `dart:_internal`, and we (surely) won't have typos in those, and users can't introduce bugs.
throw StateError(Doesn't sound like a *state* error.
A state error means an object is currently in a state where the requested functionality is not supported, but could at other times allow it.
I'm not sure which object/state this state error is referring to, and what is wrong with it.
Could it be reported as a compilation error or context relevant exception?
A format exception?
(Generally `ArgumentError`, `StateError` and `UnsupportedError` are used to report that the caller of a method has made a mistake. Here the mistake is not in the call to `_resolveRedirection`, it's in the input, which the caller is not expected to check before calling. So `FormatException` would be an option that matches.)
throw StateError('$target is redirecting to $redirectionTarget but 'Also not a `StateError`. Not an error *in this call*, so something that should probably be reported as a compilation error by the framework doing the lowering.
throw StateError('$target is redirecting to $redirectionTarget but 'Super-nit: Comma before "but".
- it is only recognized when applied to methods declared in `dart:internal`;... applied to static methods ...?
(I guess it's ignored on everything that is not a static function, but then it's actually not recognized on non-static methods.)
- it only rewrites static invocations of those methods;Aka: Not tear-offs.
(Can the invocation be, fx, a static getter invocation, or only function declarations?)
external int foo(int x);(Should not introduce public names in patches. I guess it's "safe" for `dart:_internal`. I'd still suggest putting it as `external` in `dart:_internal` itself, and provide a patch for it on all platforms, even if most throw `UnsupportedError`. Then it's clear from the source of `dart:internal` which members it has.)
import "dart:collection" show CollectionInternals;No `CollectionInternals` any more.
/** Contents of the current container being built, or null if not building a(Do consider switching to `///` comments at some point.)
List? currentContainer;Consider making this an explicit `List<dynamic>`.
(Just to be explicit.)
final kvPairs = unsafeCast<List>(currentContainer);Name it `keyValuePairs`.
Consider `assert(keyValuePairs.length.isOdd);`
if (reviver case final reviver?) {Are you absolutely sure this is as efficient as:
```dart
var revivier = this.revivier;
if (reviever != null) {
```
on all platforms?
(If so, I'm happy. If not, let's keep the simple code.)
value = createMapFromKeyValueListUnsafe<String, dynamic>((If we only ever use this method for `<String, dynamic>`, then it could be named something with `Json` and not need to take type argumens, or unsafely cast its values).
internal.unsafeCast<V>(keyValuePairs[i + 1]);This call calls `operator []=(K key, V value)` call which is coviariant by generics. Will that do `K`/`V` checks on entry, or are those optimized away when the call is on `this` (or the receiver type is otherwise known to be exact)?
If I read `[]=` correctly, it ends up copying the key/value pairs
into `_data` in exactly the same order (unless the original JSON had duplicate keys, which it never does).
Could you just take the backing array of `keyValuePairs` and use it as `_data`, and then compute the hashes and validate that there are no duplicate entries. (If there is, move the value to the earlier one if and tombstone the latter, but again, that never hapens in practice.)
Or at least do `_data.setRange(0, keyValuePairs.length, keyValuePairs);` and then do what I suggest above?
}Alternatively, we could keep the current behavior of the JSON parser, storing maps for maps, and just expose a `_Map._unsafeSet(Map map, Object? key, Object? value)` which adds the entry to the map, like the `this[...] = ...` call above, without allocating an intermediate list.
/// Token type for sharing private functionality between core libraries.Lasse NielsenCurrently the only way to share private functionality between two different core libraries (e.g. dart:collection and dart:convert) without exposing it to the users is to move implementation into a private core library and then re-export pieces of it through core libraries.
Doing this with something like `dart:collection` to expose an efficient private method for creating a `Map` from the list key-value pairs is a big refactoring because `dart:collection` has a huge API surface.
So instead I came up with this _hack_. An alternative I considered is doing something like
```
// dart:_internal// On VM this would redirect `method` to `_privateMethod` which normally can't be
// invoked.
@pragma('vm:redirect', ['dart:collection', '_privateMethod']);
external void method();
```@l...@google.com Lasse, do you have any opinions on this?
Slava EgorovYep. Don't like visible hacks. (No matter how clever, the goal is to not expose any public names to users.)
I like the idea of allowing platform libraries to redirect to each other.
(I've been very tempted to just combine all the core libraries into on `dart:_impl` and export the individual libraries from there. It's not like they're not strongly connected anyway.)I'll think about it and see if I can find an invisible alternative.
(Maybe we can have some small shared stub in `dart:_internal` that classes can opt in to being compatible with in some way. I hope, but no concrete idea yet.)
Implemented pragma based solution instead.
Copying all of `dart:collection` into `dart:_collection` and exporting it from `dart:collection`, minus anything we don't want to share, would be a valid approach. Maybe we should just do it for all the core libraries, once and for all.
Related details
- Martin Kustermann
- Slava Egorov
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 6 comments![]( "Open in Gerrit")
TreeNode visitLibrary(Library node) {We can remove this `isCoreLibrary` specific code.
If only `dart:_internal` can declare externals with redirecting pragmas, then we're already guaranteed that all callers are in `dart:*` libraries - because one cannot really import `dart:_internal` anywhere else
for (var annotation in target.annotations) {nit: final
if (reviver case final reviver?) {Are you absolutely sure this is as efficient as:
```dart
var revivier = this.revivier;
if (reviever != null) {
```
on all platforms?
(If so, I'm happy. If not, let's keep the simple code.)
Much prefer lasses code than `if (case final reviver?)`
I wouldn't have guessed that `if (reviver case final reviver?)` would effectively be a `reviver != null` check - in fact the `reviver?` would indicate to me we allow matching null). This new pattern match syntax is IMHO overused and often makes code much less readable!
int size = _roundUpToPowerOfTwo(keyValuePairs.length);`assert(keyValuePairs.length % 2 == 0)`
Would it make sense to check for `keyValuePairs.length == 0` and do something faster?
internal.unsafeCast<V>(keyValuePairs[i + 1]);This call calls `operator []=(K key, V value)` call which is coviariant by generics. Will that do `K`/`V` checks on entry, or are those optimized away when the call is on `this` (or the receiver type is otherwise known to be exact)?
If I read `[]=` correctly, it ends up copying the key/value pairs
into `_data` in exactly the same order (unless the original JSON had duplicate keys, which it never does).Could you just take the backing array of `keyValuePairs` and use it as `_data`, and then compute the hashes and validate that there are no duplicate entries. (If there is, move the value to the earlier one if and tombstone the latter, but again, that never hapens in practice.)
Or at least do `_data.setRange(0, keyValuePairs.length, keyValuePairs);` and then do what I suggest above?
👍 to lasse's 2 comments
We can steal the backing store of the array and use existing logic to create the index, see:
```
void _createIndex() {
final size =
_roundUpToPowerOfTwo(max(_data.length, _HashBase._INITIAL_INDEX_SIZE));
final newIndex = new Uint32List(size);
final hashMask = _HashBase._indexSizeToHashMask(size);
assert(_hashMask == hashMask);
for (int j = 0; j < _usedData; j += 2) {
final key = _data[j] as K;final fullHash = _hashCode(key);
final hashPattern = _HashBase._hashPattern(fullHash, hashMask, size);
final d =
_findValueOrInsertPoint(key, fullHash, hashPattern, size, newIndex);
// We just allocated the index, so we should not find this key in it yet.
assert(d <= 0);
final i = -d;
assert(1 <= hashPattern && hashPattern < (1 << 32));
final index = j >> 1;
assert((index & hashPattern) == 0);
newIndex[i] = hashPattern | index;
}
// Publish new index, uses store release semantics.
_index = newIndex;
}
```
Alternatively, we could keep the current behavior of the JSON parser, storing maps for maps, and just expose a `_Map._unsafeSet(Map map, Object? key, Object? value)` which adds the entry to the map, like the `this[...] = ...` call above, without allocating an intermediate list.
That will cause unnecessary work if the map has to resize while json parser is adding more properties. By collecting all key/values first, we ensure that we have to do hashing & index creation only once.
Related details
- Lasse Nielsen
- Slava Egorov
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
TreeNode visitLibrary(Library node) {We can remove this `isCoreLibrary` specific code.
If only `dart:_internal` can declare externals with redirecting pragmas, then we're already guaranteed that all callers are in `dart:*` libraries - because one cannot really import `dart:_internal` anywhere else
I like defense in depth. (I have once removed what I thought was one of two things preventing some possible misuse, only to discover that someone else had removed the other.)
It's not an expensive check, I'd keep it.
Related details
- Martin Kustermann
- Slava Egorov
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Adams (Gerrit)
Stephen Adams added 2 comments![]( "Open in Gerrit")
I would like the redirect to be solved at the language level.
I have needed this kind of thing for other runtimes and ended up adding a new `dart:_foo` library.
Failing that, can the transform be refactored so it is easy to use from js_runtime, js_dev_runtime etc?
int size = _roundUpToPowerOfTwo(keyValuePairs.length);`assert(keyValuePairs.length % 2 == 0)`
Would it make sense to check for `keyValuePairs.length == 0` and do something faster?
nit: `assert(keyValuePairs.length.isEven)`
Related details
- Martin Kustermann
- Slava Egorov
Code-Review
Commit-Message-Has-TEST
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")
I would like the redirect to be solved at the language level.
I have needed this kind of thing for other runtimes and ended up adding a new `dart:_foo` library.Failing that, can the transform be refactored so it is easy to use from js_runtime, js_dev_runtime etc?
There's a pattern we use in dart2wasm that allows having internally-public top-level members that are not available to users.
We implement internal types in underscored libraries, like `dart:_string`, `dart:_boxed_int`, `dart:_typed_data`.
Since underscored libraries are not available to the users, these libraries can export unsafe things at the top-level that accesses internals of the types. These top-level things can then be used by other underscored libraries.
This design worked great for dart2wasm, we basically have unrestricted access to internals of internal data structures in the entire standard library, without exposing anything extra to users.
Since `compact_hash.dart` is a part of a public library (`dart:collection`) this can't be done for `_populateUnsafe` (that would mean injecting a public member to the library).
So for dart2wasm we will either need to support the `vm:redirect` pragma, or copy `compact_hash.dart` and make it a libarary (e.g. `dart:_compact_hash`) and add a public member `createMapFromKeyValueListUnsafe`.
I'm not sure if we can avoid copying the file here. Maybe we can have something like:
```
wasm_common:
libraries:
_compact_hash:
uri: _internal/vm_shared/lib/compact_hash.dart
patches:
- _internal/wasm/lib/compact_hash_public_members.dart
```
where `compact_hash_public_members.dart` will just add a public top-level `populateUnsafe` member.
Related details
- Martin Kustermann
- Slava Egorov
- Stephen Adams
Code-Review
Commit-Message-Has-TEST
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")
}I'm not suggesting changing it, but I'm just curious if this `finally` block is necessary. I'd think that when the call site is done with one library and visits another it calls `visitLibrary` as the first thing, which will update `inCoreLibrary`. Is this not the case? Can this class visit a member (or another kind of AST node) without visiting its library first?
Lasse Nielsen (Gerrit)
Lasse Nielsen added 1 comment![]( "Open in Gerrit")
external Map<K, V> createMapFromKeyValueListUnsafe<K, V>(Consider adding another VM-specific internal library for this, instead of stuffing everything into `dart:_internal`.
Patch files should generally not add public members that are not in the cross-platform source.
Slava Egorov (Gerrit)
Slava Egorov added 22 comments![]( "Open in Gerrit")
Removed redirection pragma. Can now access necessary internals via `dart:_compact_hash` publics instead.
Lasse NielsenWe can remove this `isCoreLibrary` specific code.
If only `dart:_internal` can declare externals with redirecting pragmas, then we're already guaranteed that all callers are in `dart:*` libraries - because one cannot really import `dart:_internal` anywhere else
I like defense in depth. (I have once removed what I thought was one of two things preventing some possible misuse, only to discover that someone else had removed the other.)
It's not an expensive check, I'd keep it.
Reverted these changes.
Use `.isScheme('dart')` instead of `.scheme == 'dart`. (It avoids allocation for `_SimpleUri`s. And is case insensitive.)
```dart
inCoreLibrary = node.importUri.isScheme('dart');
```
Reverted these changes.
I'm not suggesting changing it, but I'm just curious if this `finally` block is necessary. I'd think that when the call site is done with one library and visits another it calls `visitLibrary` as the first thing, which will update `inCoreLibrary`. Is this not the case? Can this class visit a member (or another kind of AST node) without visiting its library first?
Reverted these changes.
(`finally` was here just to keep the state consistent even if we throw an exception and then catch and resume traversal - not that it would actually ever happen, but just in case.)
// Resolve redirection supplied via vm:redirect pragma if applicable.Maybe worth mentioning that it always stores a value in the `_redirectionCache`. If there is no redirection, it stores the target itself, so it's cheaper to look up a second time.
(Or insert a comment line in the code saying that. I had to read the code twice to notice that detail, which I think is fairly important.)
Reverted these changes.
for (var annotation in target.annotations) {Slava Egorovnit: final
Reverted these changes.
final [libraryUri, functionName] = targetName.split('#');This pattern match throws if the option string has more or less than one `#`. I'm not sure throwing is the best way to report an error.
Obviously that should never happen because we only end here for declarations inside `dart:_internal`, and we (surely) won't have typos in those, and users can't introduce bugs.
Reverted these changes.
Doesn't sound like a *state* error.
A state error means an object is currently in a state where the requested functionality is not supported, but could at other times allow it.I'm not sure which object/state this state error is referring to, and what is wrong with it.
Could it be reported as a compilation error or context relevant exception?
A format exception?(Generally `ArgumentError`, `StateError` and `UnsupportedError` are used to report that the caller of a method has made a mistake. Here the mistake is not in the call to `_resolveRedirection`, it's in the input, which the caller is not expected to check before calling. So `FormatException` would be an option that matches.)
Reverted these changes.
throw StateError('$target is redirecting to $redirectionTarget but 'Also not a `StateError`. Not an error *in this call*, so something that should probably be reported as a compilation error by the framework doing the lowering.
Reverted these changes.
throw StateError('$target is redirecting to $redirectionTarget but 'Super-nit: Comma before "but".
Reverted these changes.
- it is only recognized when applied to methods declared in `dart:internal`;... applied to static methods ...?
(I guess it's ignored on everything that is not a static function, but then it's actually not recognized on non-static methods.)
Reverted these changes.
- it only rewrites static invocations of those methods;Aka: Not tear-offs.
(Can the invocation be, fx, a static getter invocation, or only function declarations?)
Reverted these changes.
(Should not introduce public names in patches. I guess it's "safe" for `dart:_internal`. I'd still suggest putting it as `external` in `dart:_internal` itself, and provide a patch for it on all platforms, even if most throw `UnsupportedError`. Then it's clear from the source of `dart:internal` which members it has.)
Reverted these changes.
import "dart:collection" show CollectionInternals;Slava EgorovNo `CollectionInternals` any more.
Done
Consider making this an explicit `List<dynamic>`.
(Just to be explicit.)
Done
Name it `keyValuePairs`.
Consider `assert(keyValuePairs.length.isOdd);`
Done
Martin KustermannAre you absolutely sure this is as efficient as:
```dart
var revivier = this.revivier;
if (reviever != null) {
```
on all platforms?
(If so, I'm happy. If not, let's keep the simple code.)
Much prefer lasses code than `if (case final reviver?)`
I wouldn't have guessed that `if (reviver case final reviver?)` would effectively be a `reviver != null` check - in fact the `reviver?` would indicate to me we allow matching null). This new pattern match syntax is IMHO overused and often makes code much less readable!
Kernel for `if (a case final a?)`
```
{
final synthesized T? #0#0 = this.{test::A::a}{T?};
{
final hoisted T a;
if(!(#0#0 == null)) {
a = #0#0{T};
{
return a;
}
}
}
}
```
so it is basically the same code.
value = createMapFromKeyValueListUnsafe<String, dynamic>((If we only ever use this method for `<String, dynamic>`, then it could be named something with `Json` and not need to take type argumens, or unsafely cast its values).
We might want to reuse this in other places.
external Map<K, V> createMapFromKeyValueListUnsafe<K, V>(Consider adding another VM-specific internal library for this, instead of stuffing everything into `dart:_internal`.
Patch files should generally not add public members that are not in the cross-platform source.
Done
int size = _roundUpToPowerOfTwo(keyValuePairs.length);Stephen Adams`assert(keyValuePairs.length % 2 == 0)`
Would it make sense to check for `keyValuePairs.length == 0` and do something faster?
nit: `assert(keyValuePairs.length.isEven)`
I doubt it makes sense to optimize for empty collections.
We can make this optimization in a separate CL.
It requires much bigger changes - because there are assumptions about the length of `data` baked into the implementation, so if you want to just steal the backing store then you need to make sure it is of the right size.
And you need to filter duplicates, so you can't just reuse `_createIndex` logic.
Martin KustermannAlternatively, we could keep the current behavior of the JSON parser, storing maps for maps, and just expose a `_Map._unsafeSet(Map map, Object? key, Object? value)` which adds the entry to the map, like the `this[...] = ...` call above, without allocating an intermediate list.
That will cause unnecessary work if the map has to resize while json parser is adding more properties. By collecting all key/values first, we ensure that we have to do hashing & index creation only once.
- Martin Kustermann
- Stephen Adams
- Ömer Ağacan
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Slava Egorov (Gerrit)
Slava Egorov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: sdk/lib/_internal/vm/lib/internal_patch.dart
Insertions: 3, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: runtime/docs/compiler/pragmas_recognized_by_compiler.md
Insertions: 0, Deletions: 35.
The diff is too large to show. Please review the diff.
```
```
The name of the file: sdk/lib/_internal/vm/lib/hash_factories.dart
Insertions: 0, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: sdk/lib/_internal/vm_shared/lib/compact_hash.dart
Insertions: 38, Deletions: 25.
The diff is too large to show. Please review the diff.
```
```
The name of the file: runtime/docs/pragmas.md
Insertions: 0, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: sdk/lib/_internal/vm/lib/convert_patch.dart
Insertions: 7, Deletions: 13.
The diff is too large to show. Please review the diff.
```
```
The name of the file: pkg/vm/lib/modular/transformations/lowering.dart
Insertions: 1, Deletions: 79.
The diff is too large to show. Please review the diff.
```
Change information
[vm/corelib] Optimize building of Maps in JSON decoder.
Instead of gradually adding key-value pairs into the Map as JSON
is being parsed collect all key values first and then allocate
the map with appropriate capacity.
Issue https://github.com/dart-lang/sdk/issues/55522
TEST=ci
- M sdk/lib/_internal/vm/lib/convert_patch.dart
- M sdk/lib/_internal/vm_shared/lib/compact_hash.dart
Code-Review: +1 by Lasse Nielsen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted Code-Review+1![]( "Open in Gerrit")
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |