-
Notifications
You must be signed in to change notification settings - Fork 348
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
perf: optimize object creation in decode
, fromJSON
and fromPartial
#457
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.
Nice!
@stephenh could you please take another look? Now all properties are initialized in the base instance, including |
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.
Nice! Great idea to just initialize all the fields while we're at it.
## [1.96.1](v1.96.0...v1.96.1) (2021-12-28) ### Performance Improvements * optimize object creation in `decode`, `fromJSON` and `fromPartial` ([#457](#457)) ([70832d3](70832d3))
🎉 This PR is included in version 1.96.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Nice. We might consider removing the base object creation ( |
Yep, at first I thought that a separate function is needed to prevent duplication (that can result in e.g. increased bundle size). But actually an object literal with default values is only needed in the |
const message = { ...basePleaseChoose } as PleaseChoose; | ||
message.signature = new Uint8Array(); | ||
const message = createBasePleaseChoose(); |
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.
Here the creator function is actually nice to have. If it creates all fields, its pretty simple.
Does it make sense to just rename createBasePleaseChoose to createEmptyPleaseChoose and leave it for the decode implementation?
@@ -32,7 +32,9 @@ export interface Child { | |||
name: string; | |||
} | |||
|
|||
const baseSimple: object = { name: '', age: 0, testField: '', testNotDeprecated: '' }; | |||
function createBaseSimple(): Simple { | |||
return { name: '', age: 0, child: undefined, testField: '', testNotDeprecated: '' }; |
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.
Nice to see this creating the correct type with all fields and avoid the unsafe cast that we had before!
Exactly. This changed a lot over the last months when I migrated many of the assignments to the ?? operator or |
By the way, consider that A downside of it is that support for |
Creating an object via object spread can be 100 times slower than creating via object literal: https://jsbench.me/0nkxobf9gt/1