Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override == and hashCode in PbMap #224

Merged
merged 7 commits into from
Apr 2, 2019
Merged

Override == and hashCode in PbMap #224

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
103434e
Override == and hashCode in PbMap
szakarias Mar 20, 2019
fba29e8
comments
szakarias Apr 1, 2019
94b9e5c
conflicts
szakarias Apr 1, 2019
3f4e34e
remove duplicate line from conflict resolving
szakarias Apr 1, 2019
e2d88d8
comments
szakarias Apr 1, 2019
3663527
formatting
szakarias Apr 1, 2019
92389c9
merge
szakarias Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions protobuf/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.13.6

* Override `operator ==` and `hashCode` in `PbMap` so that two `PbMap`s are equal if they have equal key/value pairs.

## 0.13.5

* Add new method `addAll` on ExtensionRegistry for more conveniently adding multiple extensions at once.
Expand Down
1 change: 1 addition & 0 deletions protobuf/lib/protobuf.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import 'dart:math' as math;
import 'dart:typed_data' show TypedData, Uint8List, ByteData, Endian;

import 'package:fixnum/fixnum.dart' show Int64;
import 'package:quiver/core.dart' show hashObjects, hash2;

part 'src/protobuf/coded_buffer.dart';
part 'src/protobuf/coded_buffer_reader.dart';
Expand Down
29 changes: 29 additions & 0 deletions protobuf/lib/src/protobuf/pb_map.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,35 @@ class PbMap<K, V> extends MapBase<K, V> {
_wrappedMap[key] = value;
}

@override
bool operator ==(other) {
if (identical(other, this)) {
return true;
}
if (other is! PbMap) {
return false;
}
if (other.length != length) {
return false;
}
if (other.hashCode != hashCode) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calculating the hash code during equality testing is slower than just doing structural equality testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much slower if the first key is not in the other map!

It is also exponential in the nesting depth of the PbMaps when the map values contain PbMaps -
you walk the maps for this hashCode call, and again in the != below.

Perhaps you can make a benchmark for PbMaps nested 20x deep to prove we avoid this problem.

return false;
}
for (final key in keys) {
if (other[key] != this[key]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other[key] could be null.
I'd add a comment that this[key] can never be null, so it is not necessary to check other.containsKey(key), since the test will fail for missing key.

As a TODO - It might be more efficient to check all the keys first, since they are usually smaller than the values.

return false;
}
}
return true;
}

int get hashCode {
return hashObjects(_wrappedMap.keys
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we should be able to share code with _FieldSet.hashCode and PbList.hashCode

.map((key) => hash2(key.hashCode, _wrappedMap[key].hashCode))
.toList(growable: false)
..sort());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just rely on the default dart map being ordered (it is a LinkedHashMap)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No of course not! - different order of insertion should still result in the same hashcode. I think we can xor the hashcodes of all the entries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hash2 takes two Objects, and calls their hashCodes.
So the calls the hashCode here are unnecessary.

hashObjects takes an Iterable of Objects and calls the hashCodes.

In effect we are doing a lot of x.hashCode.hashCode.
At best that seems inefficient, and may result in a poor hashCode if the underlying combiner doesn't have good dispersal.
At least we can fix the two calls here.

}

@override
void clear() {
if (_isReadonly)
Expand Down
3 changes: 2 additions & 1 deletion protobuf/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: protobuf
version: 0.13.5
version: 0.13.6
author: Dart Team <misc@dartlang.org>
description: >
Runtime library for protocol buffers support.
Expand All @@ -9,6 +9,7 @@ environment:
sdk: '>=2.0.0-dev.17.0 <3.0.0'
dependencies:
fixnum: '>=0.9.0 <0.11.0'
quiver: ^2.0.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we do not get further dependencies if we can avoid it. I think it is better to duplicate it.
Hopefully we can have a resolution of dart-lang/sdk#11617 someday, and use core library functionality.

dev_dependencies:
test: '>=1.2.0'
benchmark_harness: any
Expand Down
4 changes: 4 additions & 0 deletions protoc_plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 16.0.3

* Add test for PbMap equality and update protobuf dependency to 0.13.6.

## 16.0.2

* Generated files now import 'dart:core' with a prefix
Expand Down
4 changes: 2 additions & 2 deletions protoc_plugin/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: protoc_plugin
version: 16.0.2
version: 16.0.3
author: Dart Team <misc@dartlang.org>
description: Protoc compiler plugin to generate Dart code
homepage: https://github.com/dart-lang/protobuf
Expand All @@ -10,7 +10,7 @@ environment:
dependencies:
fixnum: ^0.10.5
path: ^1.0.0
protobuf: ^0.13.4
protobuf: ^0.13.6
dart_style: ^1.0.6

dev_dependencies:
Expand Down
22 changes: 22 additions & 0 deletions protoc_plugin/test/map_field_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ library map_field_test;

import 'dart:convert';

import 'package:protobuf/protobuf.dart';
import 'package:test/test.dart';

import '../out/protos/map_field.pb.dart';
Expand Down Expand Up @@ -212,6 +213,27 @@ void main() {
_expectEmpty(testMap);
});

test(
'PbMap` is equal to another PbMap with equal key/value pairs in any order',
() {
TestMap t = TestMap()
..int32ToStringField[2] = 'test2'
..int32ToStringField[1] = 'test';
TestMap t2 = TestMap()
..int32ToStringField[1] = 'test'
..int32ToStringField[2] = 'test2';

PbMap<int, String> m = t.int32ToStringField;
PbMap<int, String> m2 = t2.int32ToStringField;

expect(t, t2);
expect(t.hashCode, t2.hashCode);

expect(m, m2);
expect(m == m2, isTrue);
expect(m.hashCode, m2.hashCode);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a negative test?

});

test('merge from other message', () {
TestMap testMap = TestMap();
_setValues(testMap);
Expand Down