Skip to content

Pass locals as arguments in endpoint rules #6131

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 4 commits into
base: master
Choose a base branch
from

Conversation

sugmanue
Copy link
Contributor

@sugmanue sugmanue commented May 23, 2025

Motivation and Context

Improvements to the compiled endpoint rules,

  1. Removes the need of a POJO object to keep track of the locally assigned values, these are now pass down directly from the caller to the calle. This reduces garbage and thus pressure on the GC. This change shaves ~30ns in simple cases, and ~100ns in non simple ones
  2. Inlines all the non-tree rules, this reduces code size ~25% (uncompressed)

Benchmarks results (see benchmark here)

Before

Benchmark                Mode  Cnt     Score    Error  Units
MyBenchmark.testComplex  avgt    5  1927.910 ± 40.728  ns/op
MyBenchmark.testSimple   avgt    5   570.353 ± 11.091  ns/op

After

Benchmark                Mode  Cnt     Score    Error  Units
MyBenchmark.testComplex  avgt    5  1804.567 ± 34.876  ns/op
MyBenchmark.testSimple   avgt    5   533.071 ± 10.559  ns/op

S3, the rule 110

Before

  • LocalState needs to be converted to builder and the build again to store s3ExpressAvailabilityZoneId
  • Each of the endpoint child rules are in different methods.
private static RuleResult endpointRule110(S3EndpointParams params, LocalState locals) {
    String s3ExpressAvailabilityZoneId = null;
    String s3ExpressAvailabilityZoneDelim = null;
    if ((s3ExpressAvailabilityZoneId = RulesFunctions.substring(params.bucket(), 7, 21, true)) != null) {
        locals = locals.toBuilder().s3ExpressAvailabilityZoneId(s3ExpressAvailabilityZoneId).build();
    } else {
        return RuleResult.carryOn();
    }
    if ((s3ExpressAvailabilityZoneDelim = RulesFunctions.substring(params.bucket(), 21, 23, true)) != null) {
        locals = locals.toBuilder().s3ExpressAvailabilityZoneDelim(s3ExpressAvailabilityZoneDelim).build();
        if ("--".equals(locals.s3ExpressAvailabilityZoneDelim())) {
            RuleResult result = endpointRule111(params, locals);
            if (result.isResolved()) {
                return result;
            }
            return endpointRule112(params, locals);
        }
    }
    return RuleResult.carryOn();
}

After

  • RulePartition partitionResult is passed down
  • The endpoint rules are now inlined instead of calling other rule methods
private static RuleResult endpointRule110(S3EndpointParams params, String region, RulePartition partitionResult) {
    String s3ExpressAvailabilityZoneId = RulesFunctions.substring(params.bucket(), 7, 21, true);
    if (s3ExpressAvailabilityZoneId != null) {
        String s3ExpressAvailabilityZoneDelim = RulesFunctions.substring(params.bucket(), 21, 23, true);
        if (s3ExpressAvailabilityZoneDelim != null) {
            if ("--".equals(s3ExpressAvailabilityZoneDelim)) {
                if (params.useFips()) {
                    return RuleResult.endpoint(Endpoint
                            .builder()
                            .url(URI.create("https://" + params.bucket() + ".s3express-fips-" + s3ExpressAvailabilityZoneId
                                    + "." + region + "." + partitionResult.dnsSuffix()))
                            .putAttribute(KnownS3ExpressEndpointProperty.BACKEND, "S3Express")
                            .putAttribute(
                                    AwsEndpointAttribute.AUTH_SCHEMES,
                                    Arrays.asList(S3ExpressEndpointAuthScheme.builder().disableDoubleEncoding(true)
                                            .signingName("s3express").signingRegion(region).build())).build());
                }
                return RuleResult.endpoint(Endpoint
                        .builder()
                        .url(URI.create("https://" + params.bucket() + ".s3express-" + s3ExpressAvailabilityZoneId + "."
                                + region + "." + partitionResult.dnsSuffix()))
                        .putAttribute(KnownS3ExpressEndpointProperty.BACKEND, "S3Express")
                        .putAttribute(
                                AwsEndpointAttribute.AUTH_SCHEMES,
                                Arrays.asList(S3ExpressEndpointAuthScheme.builder().disableDoubleEncoding(true)
                                        .signingName("s3express").signingRegion(region).build())).build());
            }
        }
    }
    return RuleResult.carryOn();
}

Modifications

Testing

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@sugmanue sugmanue marked this pull request as ready for review May 23, 2025 22:26
@sugmanue sugmanue requested a review from a team as a code owner May 23, 2025 22:26
Copy link

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.

1 participant