Skip to content

Conversation

@slandelle
Copy link
Contributor

Motivation:

When multiple NameResolvers are created, the Classloader is scanned every time trying to figure out if the Platform is Android. This expensive work could be done only once.

Modification:

Cache isAndroid resolution in a constant.

Result:

Less expensive multiple NameResolvers instantiation.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 5, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: slandelle / name: Stephane Landelle (3a1dc9e)

@slandelle slandelle force-pushed the master branch 4 times, most recently from 3ee9873 to 2a8b3d2 Compare July 5, 2023 08:08
@slandelle slandelle changed the title Resolve isAndroid only once on class loading core,grpclb: Resolve isAndroid only once on class loading Jul 5, 2023
Motivation:

When multiple NameResolvers are created, the Classloader is scanned every time trying to figure out if the Platform is Android. This expensive work could be done only once.

Modification:

Cache isAndroid resolution in a constant.

Result:

Less expensive multiple NameResolvers instantiation.
@slandelle
Copy link
Contributor Author

@grpc-google-owners Any idea why the CI has been stuck for 13 hours?

@temawi
Copy link
Contributor

temawi commented Jul 6, 2023

Is there a performance issue you are trying to resolve with this optimization? This would potentially save some "handful" of calls to get the class loader and a class at channel creation time, but those are not on any time sensitive RPC call path. I'm just trying to justify the, albeit slight, increase in code complexity.

@temawi temawi self-requested a review July 6, 2023 16:59
@temawi temawi added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jul 6, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jul 6, 2023
@temawi temawi added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Jul 6, 2023
@ejona86 ejona86 removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Jul 6, 2023
@slandelle
Copy link
Contributor Author

slandelle commented Jul 6, 2023

@temawi our use case is non standard I reckon 😄

At Gatling, we're currently working on supporting gRPC load tests.
In the case of trying so simulate a large fleet of user-agents (eg a fleet of android apps), we need to create a large amount of ManagedChannels as we really want 1 HTTP/2 connection per virtual user so the traffic against the endpoint is realistic. In this case, isAndroid is one of the hotspots we've spotted.

@temawi
Copy link
Contributor

temawi commented Jul 6, 2023

@slandelle thanks for the background, I'll go ahead and merge this. I hope it will be helpful to you guys!

@temawi temawi merged commit 1e30cb6 into grpc:master Jul 6, 2023
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

LGTM

@slandelle
Copy link
Contributor Author

Thanks guys! We've spotted several other hotspots so we'll definitely provide some other PRs in a few weeks.
Cheers

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants