-
Notifications
You must be signed in to change notification settings - Fork 224
BDD Endpoint Codegen #4433
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
base: main
Are you sure you want to change the base?
BDD Endpoint Codegen #4433
Conversation
Some/most of these will be throaway like setting "requireEndpointResolver": false in build.gradle.kts.
Still lots of missing pieces
Condidtions generate, but not completely correctly. There are some compilation errors, mostly around Options where primitive types are expected. Coalesce works and is inlined.
Still a bug in Booleq and Stringeq with refs vs owned types.
TODO: impl results and fix the evaluate_bdd impl
Still need to add the tests
failures:
config::endpoint::test::test_134
config::endpoint::test::test_135
config::endpoint::test::test_139
config::endpoint::test::test_246
The test still fails to compile, but it is on the actual test code, not the generated endpoint code.
ce9b0e0 to
5c8ed06
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
It was missing for some reason. I stole the model from smithy-java so they might have done some processing of it. Add StreamingBlob shape to S3 model
6dab86e to
54a52cb
Compare
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
aajtodd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Nice work.
Mostly minor comments outside of existing TODO's and things we've already discussed re:testing.
| return rules.getBuiltIn(builtIn) | ||
| val endpointBddTrait = idx.getEndpointBddTrait(serviceShape) | ||
| val rules = idx.endpointRulesForService(serviceShape) | ||
| if (endpointBddTrait != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/style: you can avoid multiple returns IIRC
return if(...) {
<value>
} else if (...){
<value>
} else {
<value>
}| * are used to generate `Result`s. This requires some cloning and unwrapping. The | ||
| * unwraps are all `unwrap_or_default`. This is safe since it will not panic, and | ||
| * thanks to the guarantees of BDD compilation we know that if a value is used to | ||
| * construct a Result it has a value, so default values will never be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we always expect a value why not just unwrap() or expect() then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should clarify this comment. The gist of it is we always expect aSome(_) from the variables that are used in constructing the result that is decided on at runtime. Variables that are not part of the actual result might be Some(_) or None. To simplify the codegen all of the variables are unwrapped and ready to use. The default values help with the cases of the variables that are None, so they don't fail on .unwrap().
I was considering attempting to move the variable unwrapping into the individual match arms to avoid doing this as a batch, but I haven't had a chance to experiment with that yet. I also worry that this might increase the code size quite a bit, since we will have multiple locations where each value would need to be unwrapped. But this is already a lot of code so it might not matter too much.
| else -> rust("${ref.name.rustName()}.to_owned()") | ||
| } | ||
| } catch (_: RuntimeException) { | ||
| // Typechecking was never invoked, default to .to_owned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we expect this exception? simply calling ref.type()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this was very counter-intuitive for me, but it can fail if the value has not been previously typeChecked: https://smithy.io/javadoc/1.62.0/software/amazon/smithy/rulesengine/language/syntax/expressions/Expression.html#type()
| fun testGenerator(): Writable = | ||
| defaultResolver()?.let { | ||
| // BDD takes priority just like in EndpointDecorator | ||
| defaultResolverBdd()?.let { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of branching logic around BDD vs fallback to endpoint rules. I'm assuming the plan is eventually we'll phase out endpoint rules and just have BDD, in which case this seems ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, when BDDs are fully published all of this branching logic can be pulled out and we would only use the BDD. I would like to keep it in at least for a few weeks after BDD models launch just so we have an easy way to flip back to the previous solution if we encounter a show stopping bug. I can add in TODOs around removing these once we are Trebuchet fully onboards all models to BDD traits.
| loop { | ||
| match current_ref { | ||
| // Result references (>= 100_000_000) | ||
| ref_val if ref_val >= 100_000_000 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe define constants for these magic numbers and terminals, etc.
| /// * `Some(R)` - Result if evaluation succeeds | ||
| /// * `None` - No match found | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub(crate) fn evaluate_bdd<'a, Cond, Params, Res: Clone, Context>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we relying on generated endpoint tests for testing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I could probably hand write some small BDD examples for unit testing the result/terminal behavior, but I think in general it will be hard to have handwritten BDD tests.
| /* | ||
| * Implementation Note: Autoderef Specialization | ||
| * | ||
| * This implementation uses autoderef specialization to dispatch to different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with autoderef specialization, maybe some links to resources about it for posterity. Also it doesn't require nightly or anything right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reference I am aware of is this blog post (which is included in the PR description, but not the comment) https://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.html I'll add it to the comment.
Also it doesn't require nightly or anything right?
It does not. Actual language support for non-deref based specialization does require nightly, so I avoided using that https://doc.rust-lang.org/beta/unstable-book/language-features/specialization.html
| raw: &'a str, | ||
| } | ||
|
|
||
| impl Default for Url<'_> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure default makes sense here either. Presumably we are abusing unwrap_or_default() or something for this? Do we not have other options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably we are abusing unwrap_or_default() or something for this?
This is exactly the reason. Agree that it is kind of a gross default (same with ARN), but these are not pub types so I think we can potentially get away with it. Addressed how this is used and a potential alternative that would avoid it in this comment.
Note: before this PR is actually merged it will need to have the S3 model reverted to the published one. Currently it has a custom S3 model with an
endpointBddTraitinstead just for CI testing/diff viewing purposes.Motivation and Context
Currently all AWS SDKs use a rules engine to resolve service endpoints before every operation. The current Endpoints 2.0 decision tree design creates significant problems that directly impact customers:
Larger SDK artifacts: S3's ruleset is 427 KB of JSON. The Rust SDK's generated resolver is 452 KB and nearly 6,000 lines of code. Customers download larger SDK packages and experience slower compilation times. Mobile and embedded applications face stricter size constraints that make AWS SDK adoption difficult.
Worse operation performance: Punishing fallback logic in decision tree evaluation requires 68 redundant condition checks for a simple S3 bucket+region endpoint. Customers experience slower operation initialization, particularly in serverless environments where cold starts are critical.
Description
This PR implements code generation for the new
endpointBddtrait, which uses Binary Decision Diagrams (BDDs) to optimize endpoint resolution. BDDs provide a more compact representation of endpoint rules compared to the existingendpointRuleSettrait, reducing binary size and improving resolution performance. More information about the new trait can be found on the Smithy website: https://smithy.io/2.0/additional-specs/rules-engine/specification.html#smithy-rules-endpointbdd-traitThe entrypoint to the code generation is in
EndpointsDecorator.ktwhere, if anendpointBddtrait exists, it is prioritized over theendpointRuleSettrait. This invokes theEndpointBddGeneratorwhich orchestrates most of the code generation. The functions in the match arms of each of the condition blocks are generated by theBddExpressionGeneratorclass. The most recent diff of this generated code is available here.The most notable new Rust code lives in the
rust-runtime/inlineable/crate:endpoint_lib/bdd_interpreter.rscontains theevaluate_bddfunction used to actually traverse the BDD DAG and extract a Result.endpoint_lib/coalesce.rsexports a newcoalesce!macro that implements the new endpoint std lib coalesce function. Note that the macro here uses a technique called autoderef-based specialization and was inspired by this blog post: https://lukaskalbertodt.github.io/2019/12/05/generalized-autoref-based-specialization.htmlDiving deeper into the code in the diff there are a few important new types to discuss:
struct BddNodecontaining thecondition_index,high_ref, andlow_ref.enum ConditionFnto represent the functions for evaluating each condition.enum ResultEndpointto represent the functions for generating each result.struct ConditionContextto represent variables that can be assigned by ConditionsFor each of
BddNode,ConditionFn, andResultEndpointthere is a constant sized array containing instances of each so they can be referenced by index:ConditionFnimplements a single function,evaluate, that takes in the modeled endpoint Params and theConditionContextfor passing around state between different condition evaluations, and a diagnostic_collector for tracking data about the evaluation.ResultEndpointalso implements a single functionto_endpointthat either returns anEndpointor aResolveEndpointErrordepending on which result index is passed. Note that index 0 corresponds to theNoMatchRuleThe final important portion of our resolver is the
evaluate_bddfunction. This function is handwritten and will work with the generated nodes, conditions, and results for each service. This function takes 4 generic types:Condfor the generated conditions,Paramsfor the modeled endpoint params,Resfor the generated results, andContextfor the shared condition context.TODO: There are a few tasks left for the BDD work to be fully complete:
endpointBddtraits. This work in being done as part of the SEP process and will come in a subsequent PR.aws_sdk_s3::config::endpoint::ResultEndpoint::to_endpointsymbol is still the largest one in my test binary. I would like to experiment with reducing the size there. This could look like splitting the match arms into separate functions, but a quick POC of that approach slightly increased the overall binary size even though it did reduce the size of the largest symbol.Testing
New minimal tests introduced for BDD, more to come as the SEP is fleshed out and I manage to compile some of the existing tests to BDD format.
Note: A few tests here are failing, I'm pretty all of those are due to differences in the S3 model I am using. I borrowed it from smithy-java and it was missing a few things (like
Bodymembers on some input/output types).Note: More codegen tests will be introduced as the SEP is finalized and more tests are converted to BDD format.
In addition to the new tests I performed various benchmarks against the S3 test suite. The raw data is too large to include here, but across the 347 tests in the S3 model the average speedup compared to the previous tree based generation was ~55%.
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.