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

chore(Spanner): Make createTransactionSelector faster #7342

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

saranshdhingra
Copy link
Contributor

This PR simply replaces the use of decodeMessage from the Serializer and uses the mergeFromJsonString and the protobuf class's setter method.

The metrics on average are:

Using decodeMessage

When createTransactionSelector is called the first time: 900 us(micros)
When createTransactionSelector is called the second time: 300 us (This is low because the Serializer caches a lot of things like proto descriptors etc).

Using mergeFromJsonString

When createTransactionSelector is called the first time: 200 us(micros)
When createTransactionSelector is called the second time: 150 us

@saranshdhingra saranshdhingra requested review from a team as code owners May 24, 2024 11:43
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Spanner API. label May 24, 2024
@saranshdhingra saranshdhingra added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 24, 2024
@saranshdhingra
Copy link
Contributor Author

Seems like protobuf's mergeFromString doesn't work with the following:
{"begin": {"readWrite": []}} whereas it works with: {"begin": {"readWrite": {}}}.

For now, closing this PR.

@bshaffer bshaffer reopened this Jun 13, 2024
@bshaffer
Copy link
Contributor

Seems like protobuf's mergeFromString doesn't work with the following:
{"begin": {"readWrite": []}} whereas it works with: {"begin": {"readWrite": {}}}.

I have made sure that the JSON string uses {} for empty arrays by providing the JSON_FORCE_OBJECT flag, but the emulator tests are still failing. This will require further investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants