-
Notifications
You must be signed in to change notification settings - Fork 185
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
Expect same message types in mergeFromMessage
#661
base: master
Are you sure you want to change the base?
Conversation
I'm also not sure which approach would be best. |
I think we should be able to benchmark this by using
Currently we have 2 somewhat large messages in benchmarks:
We can add a benchmark that measures Note: these benchmarks will be merged to benchmarks/ soon: #698 |
I added a new benchmark to measure AOT, master: protobuf_deep_copy(RunTimeRaw): 625.190625 us. AOT, this PR: protobuf_deep_copy(RunTimeRaw): 475.6320665083135 us. Diff: -24% JS, master: protobuf_deep_copy(RunTimeRaw): 738.3763837638377 us. JS, this PR: protobuf_deep_copy(RunTimeRaw): 639.297124600639 us. Diff: -8% This should improve |
We should also try implementing a |
This is done in #742 which halves AOT deepCopy benchmark runtime. |
Sigurd asked about whether it makes sense to allow merging messages with different types. I think it doesn't, and I tried adding this to the merge method and tested the code internally: if (!identical(_meta, other._meta)) {
throw 'Merging messages with different builder infos';
} To my surprise, this doesn't break anything. So I will simplify the merge implementation in this PR and assume that we're always merging messages of same types. The code will be simpler, and with the same performance wins. |
…with non-extension fields
ffa6703
to
d295939
Compare
_mergeFromMessage
_mergeFromMessage
mergeFromMessage
This PR refactors
GeneratedMessage.mergeFromMessage
to check that messagesbeing merged have the same type.
mergeFromMessage
now throws anArgumentError
if the messages have different types.Type check is implemented by comparing message
BuilderInfo
s for pointerequality.
This allows simplifying the merge logic and avoiding some checks. The new
version is ~7% faster:
(Using deepCopy benchmark as deepCopy is implemented as merging empty message
with the message being copied)
Before:
After:
_validateField
at the end of_mergeField
removed: we already have a validmessage so the field value must be valid for the message type.
_validateField
also checks that the message is writable, which is also unnecessary as we check
it in
_mergeFromMessage
.Fixes #60