Skip to content

Conversation

schlosna
Copy link
Contributor

@schlosna schlosna commented Aug 20, 2025

Before this PR

In services that make high throughput RPCs with small responses (think similar clients of timestamps, locks, authorization results, etc. as #3114 ), String#charAt & com.google.common.base.Splitter#splitToList pop up as hot methods & allocators due to DialogueFeignClient.FeignEndpoint#renderPath query parameter key=value parsing.

image

After this PR

==COMMIT_MSG==
Use vectorized indexOf to improve query parameter parsing and reduce allocation overhead.
==COMMIT_MSG==

Possible downsides?

@changelog-app
Copy link

changelog-app bot commented Aug 20, 2025

Generate changelog in changelog/@unreleased

Type (Select exactly one)

  • Feature (Adding new functionality)
  • Improvement (Improving existing functionality)
  • Fix (Fixing an issue with existing functionality)
  • Break (Creating a new major version by breaking public APIs)
  • Deprecation (Removing functionality in a non-breaking way)
  • Migration (Automatically moving data/functionality to a new system)

Description

Use vectorized indexOf to improve query parameter parsing and reduce allocation overhead.

Check the box to generate changelog(s)

  • Generate changelog entry

@changelog-app
Copy link

changelog-app bot commented Aug 20, 2025

Successfully generated changelog entry!

What happened?

Your changelog entries have been stored in the database as part of our migration to ChangelogV3.

Need to regenerate?

Simply interact with the changelog bot comment again to regenerate these entries.

@schlosna schlosna requested review from pkoenig10, aldexis, a team and Copilot August 20, 2025 20:25
Copilot

This comment was marked as outdated.

@schlosna schlosna requested a review from Copilot August 20, 2025 20:32
@palantir palantir deleted a comment from Copilot AI Aug 20, 2025
@palantir palantir deleted a comment from Copilot AI Aug 20, 2025
Copilot

This comment was marked as outdated.

Copilot

This comment was marked as outdated.

@schlosna schlosna requested a review from Copilot August 21, 2025 14:16
Copilot

This comment was marked as outdated.

@schlosna schlosna requested a review from Copilot August 22, 2025 02:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes query parameter parsing in DialogueFeignClient.FeignEndpoint#renderPath to improve performance in high-throughput RPC scenarios. The optimization replaces Guava's Splitter with vectorized indexOf operations to reduce allocation overhead and improve parsing efficiency.

  • Replaces QUERY_VALUE_SPLITTER with direct indexOf operations for key=value parsing
  • Adds comprehensive test coverage for query parameter edge cases including empty values and malformed parameters
  • Maintains existing error handling behavior while improving performance

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
DialogueFeignClient.java Replaces Guava Splitter with direct string operations for query parameter parsing
JaxRsClientDialogueEndpointTest.java Adds test coverage for query parameter parsing edge cases and error conditions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

List<String> keyValuePair = QUERY_VALUE_SPLITTER.splitToList(querySegment);
if (keyValuePair.size() != 2) {
int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR);
if (equalsIndex > 0) {
Copy link
Preview

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The condition equalsIndex > 0 excludes valid query parameters that start with '=' (empty key). Based on the test cases, it appears '=' should be handled as a valid parameter with empty key and empty value. The condition should be equalsIndex >= 0 to handle this case.

Suggested change
if (equalsIndex > 0) {
if (equalsIndex >= 0) {

Copilot uses AI. Check for mistakes.

@@ -417,14 +418,17 @@ public void renderPath(ListMultimap<String, String> params, UrlBuilder url) {
if (queryParamsStart != -1) {
String querySegments = trailing.substring(queryParamsStart + 1);
for (String querySegment : QUERY_SPLITTER.split(querySegments)) {
List<String> keyValuePair = QUERY_VALUE_SPLITTER.splitToList(querySegment);
if (keyValuePair.size() != 2) {
int equalsIndex = querySegment.indexOf(QUERY_KEY_VALUE_SEPARATOR);
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to ensure that there aren't more than 2 segments.

Comment on lines +422 to +426
if (equalsIndex > 0) {
String key = querySegment.substring(0, equalsIndex);
String value = querySegment.substring(equalsIndex + 1);
url.queryParam(urlDecode(key), urlDecode(value));
} else {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer if we wrote this as if-throw to avoid additional nesting in the non-exceptional case.

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