Skip to content

Transcoding optimizations #173

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

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

Conversation

zZHorizonZz
Copy link
Member

@zZHorizonZz zZHorizonZz commented May 16, 2025

Motivation:

This PR optimizes some transcoding utility classes and added more test to path matcher test class.

@zZHorizonZz zZHorizonZz changed the title Transcoding optimazations Transcoding optimizations May 16, 2025
@vietj vietj added this to the 5.0.1 milestone May 16, 2025
@vietj
Copy link
Member

vietj commented May 17, 2025

I think we could write a JMH benchmark for this

@zZHorizonZz
Copy link
Member Author

zZHorizonZz commented May 17, 2025

I have been thinking about benchmarks for gRPC in general because there are TechEmpower tests which are testing Vert.x, but we don't really have a comparison of vertx-grpc with, for example, the grpc-ecosystem version.

That beign said, I don't think this is a major optimization because in general this only reduces some unnecessary calls, and in PercentEncoding it just puts the reserved chars to BitSet away from switch (which might make it a bit slower than using switch, but at the same time this way it is more readable, I think). Also if there was something for percent encoding already I would use that but I didn't really find anything in google lib it only provides escapers and not unescapers.

@vietj
Copy link
Member

vietj commented May 17, 2025

here I am thinking about microbenchmarking the convertion without gRPC transport itself

@zZHorizonZz
Copy link
Member Author

Yeah, I can look into that 👍

@vietj
Copy link
Member

vietj commented May 17, 2025

we might be surprised by having extra bytes copy happening

@zZHorizonZz
Copy link
Member Author

zZHorizonZz commented May 18, 2025

Well, I have added some basic benchmarks (I used Vertx core for the setup) and I have run the tests with old and the new. For MessageWeaver class, the new version is faster, but PercentEncoding class is seems slower. I will try to look into that.

@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch 2 times, most recently from 5b68f47 to e7a1874 Compare May 18, 2025 20:51
@zZHorizonZz zZHorizonZz requested a review from vietj May 22, 2025 04:39
@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch 2 times, most recently from 4649b2c to 9b5f7b8 Compare May 23, 2025 21:08
@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch from 9b5f7b8 to 20c62e2 Compare May 30, 2025 10:19
return false;
}
}
// RFC 3986 reserved characters: gen-delims and sub-delims
Copy link
Member

Choose a reason for hiding this comment

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

where is this copied from ?

Copy link
Member Author

@zZHorizonZz zZHorizonZz Jun 3, 2025

Choose a reason for hiding this comment

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

It is in referenced in line underneath it, basically RFC 6570 and RFC 3986 are the same, in terms of reserved characters as RFC 6570 builds upon RFC 3986 I think?, but I find RFC3986 having little bit more explanation.

Copy link
Member

Choose a reason for hiding this comment

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

did you implement it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch from 20c62e2 to a4ce83b Compare June 4, 2025 15:32
@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch from 6feb69c to 50dfd79 Compare June 16, 2025 04:46
@vietj vietj modified the milestones: 5.0.1, 5.0.2 Jun 24, 2025
@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch from 50dfd79 to 6aaae6c Compare July 4, 2025 04:52
@zZHorizonZz zZHorizonZz force-pushed the transcoding-optimazations branch from 6aaae6c to 79c1e69 Compare July 30, 2025 04:18
@vietj vietj removed this from the 5.0.2 milestone Aug 5, 2025
@vietj vietj added this to the 5.0.3 milestone Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants