Skip to content
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

feat: xml serde reduction #1108

Merged
merged 7 commits into from
Dec 18, 2023
Merged

feat: xml serde reduction #1108

merged 7 commits into from
Dec 18, 2023

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Dec 11, 2023

This PR implements a class called StringStore, which allocates shortened variables for any given string.

The basic idea is a transform of something like:

const body = '<?xml version="1.0" encoding="UTF-8"?>';
...
const body = '<?xml version="1.0" encoding="UTF-8"?>';
...
const body = '<?xml version="1.0" encoding="UTF-8"?>';
// in multiple functions

to

const body = _ve;
...
const body = _ve;
...
const body = _ve;
...
// usages only appear in functions so it's ok to define this at the bottom of the file.
const _v2 = '<?xml version="1.0" encoding="UTF-8"?>';

Real example from S3:

const headers = map({}, isSerializableHeaderValue, {
    "content-type": "application/xml",
    "x-amz-acl": input.ACL,
    "content-md5": input.ContentMD5,
    "x-amz-sdk-checksum-algorithm": input.ChecksumAlgorithm,
    "x-amz-grant-full-control": input.GrantFullControl,
    "x-amz-grant-read": input.GrantRead,
    "x-amz-grant-read-acp": input.GrantReadACP,
    "x-amz-grant-write": input.GrantWrite,
    "x-amz-grant-write-acp": input.GrantWriteACP,
    "x-amz-expected-bucket-owner": input.ExpectedBucketOwner,
});
const headers: any = map({}, isSerializableHeaderValue, {
    "content-type": "application/xml",
    [_xaa]: input[_ACL],
    [_cm]: input[_CMD],
    [_xasca]: input[_CA],
    [_xagfc]: input[_GFC],
    [_xagr]: input[_GR],
    [_xagra]: input[_GRACP],
    [_xagw]: input[_GW],
    [_xagwa]: input[_GWACP],
    [_xaebo]: input[_EBO],
});
// variables are defined at the end

To answer some potential questions:

  • doesn't this make the code hard to read?

    • Yes. At least, you can hover over the variables to see what they are 😢. I believe users would prefer the reduced code size over readability of the protocol files.
  • is the variable allocation deterministic?

    • Yes. Variables are named with a preference for the word initials of the given string. This makes the variables slightly longer than they need to be, but reduces variable thrashing with model updates.
  • does this affect tree-shaking?

    • no. I tested with esbuild and only those variables being used are included in the bundle.
  • what is the effect on protocol file sizes?

  • what about strings that only appear once?

    • within the codegen program's current framework, it is not known when allocating a variable how many times it will be used. It would require a second pass to inline variables with only 1 usage. This change increases char-count for strings that only appear once. However, in practice (spot-checking feat: xml codegen reduction aws/aws-sdk-js-v3#5566) most strings appear multiple times.

@kuhe kuhe force-pushed the feat/xml branch 8 times, most recently from 8e99897 to d932dad Compare December 15, 2023 17:07
@kuhe kuhe marked this pull request as ready for review December 15, 2023 17:43
@kuhe kuhe requested review from a team as code owners December 15, 2023 17:43
@kuhe kuhe requested a review from gosar December 15, 2023 17:43
@@ -673,7 +677,9 @@ private void generateOperationRequestSerializer(
// Get the hostname, path, port, and scheme from client's resolved endpoint.
// Then construct the request from them. The client's resolved endpoint can
// be default one or supplied by users.
writer.write("const {hostname, protocol = $S, port, path: basePath} = await context.endpoint();", "https");

writer.addImport("requestBuilder", "rb", TypeScriptDependency.SMITHY_CORE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

requestBuilder is from #1107

@kuhe kuhe merged commit 3ad9987 into smithy-lang:main Dec 18, 2023
7 checks passed
@kuhe kuhe deleted the feat/xml branch December 18, 2023 20:33
This pull request was closed.
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.

4 participants