-
Notifications
You must be signed in to change notification settings - Fork 184
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.
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
Implement deepCopy as copying, instead of merge #742
base: master
Are you sure you want to change the base?
Conversation
|
||
final copy = _FieldSet._( | ||
message, | ||
null, // TODO: event plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a LDAP or an issue number in todos.
null, // TODO: event plugin | |
null, // TODO(omersa): event plugin |
values, | ||
null, | ||
_unknownFields?.deepCopy(freeze: freeze), | ||
freeze, // TODO: Can we use `_frozenState` here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
|
||
PbList<E> deepCopy({bool freeze = false}) { | ||
// TODO: We know the capacity, is it possible to allocate a list with the right size? | ||
final wrappedList = <E>[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could possibly use List.generate
to fill in the right values from the start, thereby avoiding the re-allocation.
This causes a lot of breakage internally. I think one of the reasons is probably that I'm assuming Since we can't have nested lists and maps, cloning of nested maps and lists will have to go through Interestingly, we assume void _markReadOnly() {
if (_isReadOnly) return;
_frozenState = true;
for (var field in _meta.sortedByTag) {
if (field.isRepeated) {
PbList? list = _values[field.index!];
if (list == null) continue;
list.freeze();
} else if (field.isMapField) {
PbMap? map = _values[field.index!];
if (map == null) continue;
map.freeze();
} else if (field.isGroupOrMessage) {
final entry = _values[field.index!];
if (entry != null) {
(entry as GeneratedMessage).freeze();
}
}
}
_extensions?._markReadOnly();
_unknownFields?._markReadOnly();
} If I'm right that the breakage here caused by users overriding list and map field types, then I'm not sure how We should just rewrite this library.. |
I managed to fix at least some of the downstream breakage by removing the assumptions in the implementation about repeated fields being The fact that we csan't assume this means freezing messages is buggy, for the reasons described in my previous comment above. Performance is still much better than merge-based implementation. I will update the PR description, and refactor the code a little bit (document, revert some of the changes). |
This version fixed some of the previous failures, but introduced new ones. |
Currently when we copy a message we create an empty message, and then merge the
copied message to the empty message.
This is not optimal as merging is a more complicated operation then copying.
This implement
deepCopy
as simple deep copy and halvesdeepCopy
benchmarkresults.
GeneratedMessage.deepCopy
also gets a optionalfreeze
argument foroptionally generating frozen messages. Previously we would have to copy and
then make another pass to mark the message as frozen.
Changes:
deepCopy
method added toPbList
,PbMap
,_ExtensionFieldSet
,UnknownFieldSet
,_FieldSet
GeneratedMessage.deepCopy
now calls_FieldSet.deepCopy
, doesn't usemerging
(I also removed generating
clone()
overrides, but I will revert it for nowand maybe do it in a separate PR)
Benchmarks:
protobuf_deep_copy(RunTimeRaw): 635.2863492063492 us.
protobuf_deep_copy(RunTimeRaw): 307.04386503067485 us.
cl/470187112