-
Notifications
You must be signed in to change notification settings - Fork 4
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
Improve omgidl deserialization performance #123
Conversation
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.
Great work on this! That definitely more of an improvement than I would've expected for this work.
Would be great to pull out the deserializer builders/cache to a separate class and write a few tests for them. Then for the class we can explain why it's necessary in the context of this performance improvement.
packages/omgidl-serialization/src/DeserializationInfoCache.test.ts
Outdated
Show resolved
Hide resolved
reader: CdrReader, | ||
options: HeaderOptions, | ||
/** The size of the struct if known (like from an emHeader). If it is known we do not read in a dHeader */ | ||
knownTypeSize?: number, | ||
flags: { knownTypeSize?: number }, |
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.
we should either join this into options
or keep as a number to avoid instantiating new object instances that need to be garbage collected.
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.
Just noticed that the value of knownTypeSize
is never used, it is only checked if it is defined. Is/was this intended?
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.
This was intended. However could be good information to have if we want to skip deserializing parts of messages in the future.
|
||
// fieldLength only used for `string` type primitives | ||
return deser(reader, headerSpecifiedLength); | ||
private readComplexNestedArray( |
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.
is readNestedArray
necessary anymore?
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.
Yes, this is used for reading typed arrays
**User-Facing Changes** Improve omgidl deserialization performance **Description** Bumps `@foxglove/omgidl-serialization` which improves deserialization performance (foxglove/omgidl#123)
Public-Facing Changes
Improve omgidl deserialization performance
Description
This PR changes the
MessageReader
to build a tree structure of deserialization info objects when constructed. When building this tree, the concrete deserializers as well as other relevant type information are looked up which avoids that this has to be done during the actual deserialization. Further changes include the removal of some temporary closures which reduces some GC time.The outcome of these changes is a performance improvement of up to 30% (depends on message type)
In the image above, left is this PR, right is the currently released version
Resolves FG-6300