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.

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

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Jan 24, 2024

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)

image
In the image above, left is this PR, right is the currently released version

Resolves FG-6300

@achim-k achim-k requested a review from snosenzo January 24, 2024 16:34
Copy link
Collaborator

@snosenzo snosenzo left a 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.

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 },
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is readNestedArray necessary anymore?

Copy link
Contributor Author

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

@achim-k achim-k merged commit 549c1f6 into main Jan 31, 2024
2 checks passed
@achim-k achim-k deleted the achim/fg-6300-omgidl-deser-perf-improvements branch January 31, 2024 11:44
achim-k added a commit that referenced this pull request Jan 31, 2024
pezy pushed a commit to pezy/studio that referenced this pull request Sep 14, 2024
**User-Facing Changes**
Improve omgidl deserialization performance

**Description**
Bumps `@foxglove/omgidl-serialization` which improves deserialization
performance (foxglove/omgidl#123)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants