Skip to content

Commit

Permalink
[vm] Map key and value iterator speedup
Browse files Browse the repository at this point in the history
Duplicates `_CompactIterable` and `_CompactIterator` to
`_CompactIterableImmutable` and `_CompactIteratorImmutable` to prevent
the `_data` field to store both modifiable and unmodifiable lists.

This speeds up iteration by about 20% (more info on the bug).

Does not use mixins for code sharing, because all the fields need to be
in the concrete classes anyway.

Closes: #48280

TEST=existing lib tests

Change-Id: I514d058d02ef3b26b475000daea3afc12f37c566
Cq-Include-Trybots: luci.dart.try:analyzer-nnbd-linux-release-try,app-kernel-linux-debug-x64-try,dart-sdk-linux-try,front-end-nnbd-linux-release-x64-try,pkg-linux-debug-try,vm-canary-linux-debug-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-checked-linux-release-x64-try,vm-kernel-linux-debug-x64c-try,vm-kernel-linux-debug-x64-try,vm-kernel-nnbd-linux-release-simarm-try,vm-kernel-optcounter-threshold-linux-release-x64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64c-try,vm-kernel-reload-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-linux-debug-simarm_x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-tsan-linux-release-x64-try,vm-kernel-tsan-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/231243
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
  • Loading branch information
dcharkes authored and Commit Bot committed Feb 3, 2022
1 parent 9df38b5 commit 33e170e
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@ class Class extends core::Object {
synthetic constructor •() → self::Class
: super core::Object::•()
;
[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3384,getterSelectorId:3385] method method([@vm.inferred-type.metadata=dart.core::Null? (value: null)] self::Enum e) → core::int
[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3396,getterSelectorId:3397] method method([@vm.inferred-type.metadata=dart.core::Null? (value: null)] self::Enum e) → core::int
return [@vm.inferred-type.metadata=!] e.{core::_Enum::index}{core::int};
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,6 @@ class ConstClass extends core::Object {
synthetic constructor •() → self::ConstClass
: super core::Object::•()
;
[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3388,getterSelectorId:3389] method method([@vm.inferred-type.metadata=dart.core::Null? (value: null)] self::ConstEnum e) → core::int
[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3400,getterSelectorId:3401] method method([@vm.inferred-type.metadata=dart.core::Null? (value: null)] self::ConstEnum e) → core::int
return [@vm.inferred-type.metadata=!] e.{core::_Enum::index}{core::int};
}
62 changes: 62 additions & 0 deletions sdk/lib/_internal/vm/lib/compact_hash.dart
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,11 @@ class _InternalImmutableLinkedHashMap<K, V> extends _HashVMImmutableBase
// Publish new index, uses store release semantics.
_index = newIndex;
}

Iterable<K> get keys =>
new _CompactIterableImmutable<K>(this, _data, _usedData, -2, 2);
Iterable<V> get values =>
new _CompactIterableImmutable<V>(this, _data, _usedData, -1, 2);
}

// Implementation is from "Hacker's Delight" by Henry S. Warren, Jr.,
Expand Down Expand Up @@ -628,6 +633,7 @@ class _CompactLinkedCustomHashMap<K, V> extends _HashFieldBase
// and checks for concurrent modification.
class _CompactIterable<E> extends Iterable<E> {
final _HashBase _table;
// dart:core#_List (sdk/lib/_internal/vm/lib/array.dart).
final List _data;
final int _len;
final int _offset;
Expand All @@ -646,6 +652,7 @@ class _CompactIterable<E> extends Iterable<E> {

class _CompactIterator<E> implements Iterator<E> {
final _HashBase _table;
// dart:core#_List (sdk/lib/_internal/vm/lib/array.dart).
final List _data;
final int _len;
int _offset;
Expand Down Expand Up @@ -677,6 +684,58 @@ class _CompactIterator<E> implements Iterator<E> {
E get current => _current as E;
}

// Iterates through _data[_offset + _step], _data[_offset + 2*_step], ...
// and checks for concurrent modification.
class _CompactIterableImmutable<E> extends Iterable<E> {
// _HashBase with _HashVMImmutableBase.
final _HashBase _table;
// dart:core#_ImmutableList (sdk/lib/_internal/vm/lib/array.dart).
final List _data;
final int _len;
final int _offset;
final int _step;

_CompactIterableImmutable(
this._table, this._data, this._len, this._offset, this._step);

Iterator<E> get iterator =>
new _CompactIteratorImmutable<E>(_table, _data, _len, _offset, _step);

int get length => _table.length;
bool get isEmpty => length == 0;
bool get isNotEmpty => !isEmpty;
}

class _CompactIteratorImmutable<E> implements Iterator<E> {
// _HashBase with _HashVMImmutableBase.
final _HashBase _table;
// dart:core#_ImmutableList (sdk/lib/_internal/vm/lib/array.dart).
final List _data;
final int _len;
int _offset;
final int _step;
final int _checkSum;
E? _current;

_CompactIteratorImmutable(
_HashBase table, this._data, this._len, this._offset, this._step)
: _table = table,
_checkSum = table._checkSum;

bool moveNext() {
_offset += _step;
if (_offset < _len) {
_current = internal.unsafeCast<E>(_data[_offset]);
return true;
} else {
_current = null;
return false;
}
}

E get current => _current as E;

This comment has been minimized.

Copy link
@gmpassos

gmpassos Feb 3, 2022

Contributor

Unnecessary casting to E. Just need to use _current!.

This comment has been minimized.

Copy link
@mraleph

mraleph Feb 3, 2022

Member

E might be a nullable type, so you can't use _current! here.

This comment has been minimized.

Copy link
@gmpassos

gmpassos Feb 3, 2022

Contributor

Understood.

Maybe _current can be declared as late E _current. I don't know which version is more efficient (as E or declared as late).

This comment has been minimized.

Copy link
@mraleph

mraleph Feb 4, 2022

Member

late E _current does not work either, because you want to reset it to null at the end of the iteration to maintain the invariant that it.current starts returning null (or throwing in NNBD) once it.moveNext() returns false.

This has been extensively discussed during dart:core NNBD migration and unfortunately _current as E is the one we had to settle with in all iterators.

This comment has been minimized.

Copy link
@gmpassos

gmpassos Feb 4, 2022

Contributor

Thanks for the response and the discussion.

It's also important to dispose any element reference once the iterator reaches the end. So inevitably _current should be set to null at the end.

}

abstract class _LinkedHashSetMixin<E> implements _HashBase {
int _hashCode(e);
bool _equals(e1, e2);
Expand Down Expand Up @@ -959,6 +1018,9 @@ class _CompactImmutableLinkedHashSet<E> extends _HashVMImmutableBase

// Returns a mutable set.
Set<E> toSet() => new _CompactLinkedHashSet<E>()..addAll(this);

Iterator<E> get iterator =>
new _CompactIteratorImmutable<E>(this, _data, _usedData, -1, 1);
}

class _CompactLinkedIdentityHashSet<E> extends _HashFieldBase
Expand Down

0 comments on commit 33e170e

Please sign in to comment.