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

perf: fromJSON returns object literal to allow v8 optimizations #463

Merged
merged 5 commits into from
Jan 6, 2022

Conversation

boukeversteegh
Copy link
Collaborator

@boukeversteegh boukeversteegh commented Jan 5, 2022

Felt like doing something useful and understood from https://github.com/stephenh/ts-proto/pull/462/files/2649b9aeb6c35779dc8294575fa70b7a35c5f79d#r778884590 that the plan was to return objects directly from fromJSON, rather than constructing a base object and assigning the fields.

But I may have been too quick, as then I found out that @aikoven is already working on this as well, so please close this PR if needed.

before:

 fromJSON(object: any): Simple {
    const message = createBaseSimple();
    message.name = isSet(object.name) ? String(object.name) : '';
    message.age = isSet(object.age) ? Number(object.age) : 0;
    return message;
  };

after:

fromJSON(object: any): Simple {
    return {
      name: isSet(object.name) ? String(object.name) : '',
      age: isSet(object.age) ? Number(object.age) : 0,
    };
 },

Copy link
Collaborator

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Great stuff

integration/batching-with-context/batching.ts Show resolved Hide resolved
integration/batching-with-context/batching.ts Show resolved Hide resolved
integration/oneof-unions/oneof.ts Show resolved Hide resolved
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

This is great! Thanks @boukeversteegh !

Fwiw I'm pretty sure @aikoven was more specifically looking at optimizing fromPartial and not necessarily fromJSON, so I'm pretty good merging this as-is. Doesn't hurt to wait for him to chime in, but I'll defer either way to you @boukeversteegh .

src/main.ts Outdated
`);
chunks.push(code`}`);

// First case, output field name
Copy link
Owner

Choose a reason for hiding this comment

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

Ah nice, I see...

Seems like this is relying on all of the oneofFields being grouped together, sequentially in the incoming messageDesc.field list.

I wouldn't be surprised if this is only true for our relatively clean *.proto test files, and that real-world proto files this may not be the case? Maybe? Not sure.

Actually I guess if the order is based on *.proto order than we should be good, so maybe this is just fine. I'm good keeping it as it is, b/c it's simpler this way, and we can see if anyone ends up reporting not-actually-in-order oneof fields as users try it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this one got me thinking for some time as well...

As far as I can reason through this, it shouldn't matter in what order the fields are delivered, and even whether there are random unrelated fields in between.

We are generating the fields in order of messageDesc.field, and the index checks (first / last) are based on a subset of the same sequence. So no matter how the order might be messed up, the first will always be the first etc. If that makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realize that the above merely guarantees correct detection of the first and last item, but if indeed somehow oneof field members were interspersed with other random fields, we would mess up the output.

However, I do wish to rely on the protobuf sdk to keep the fields in the declared order, and in that case, it should all work out.

I've changed this part of the code a bit to be more readable, hope that helps.

@aikoven
Copy link
Collaborator

aikoven commented Jan 6, 2022

Looks great!

I didn't yet start working on object literals for fromPartial, so no conflict here.

@boukeversteegh boukeversteegh merged commit 5fcd05b into main Jan 6, 2022
@boukeversteegh boukeversteegh deleted the chore/from-json-inline branch January 6, 2022 10:00
stephenh pushed a commit that referenced this pull request Jan 6, 2022
## [1.97.2](v1.97.1...v1.97.2) (2022-01-06)

### Performance Improvements

* fromJSON returns object literal to allow v8 optimizations ([#463](#463)) ([5fcd05b](5fcd05b))
@stephenh
Copy link
Owner

stephenh commented Jan 6, 2022

🎉 This PR is included in version 1.97.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants