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: generate unified error dispatcher #1150

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Jan 31, 2024

Currently, output deserializers generate one "Error Dispatcher" per operation output. This may mean hundreds of these dispatcher functions (given that many commands) when the set of modeled errors tends to only be 5 to 10 in number.

This PR merges all error dispatchers into one function per service.

Comparison Metrics:

  • Web: bundle size of S3 is ~400 bytes larger when fully gzipped/minified as in a production environment. Total of ~160kb -> ~161kb.

  • Node.js: install size of dist-cjs

    • S3, 1020kb -> 976kb
    • Lambda, 784kb -> 700kb
    • EC2, 6.5mb -> 5.8mb
    • SageMaker, 3.1mb -> 2.6mb
  • What are the outlier AWS services with the highest count of distinct modeled exceptions? and what is the average count of modeled exceptions per service?

    • cloudtrail, 83

    • codecommit, 188 (600 line de_CommandError fn)

      • regardless of that, codecommit dist-cjs also gets smaller 1.1mb -> 896kb
    • codedeploy, 110

    • config-server, 53

    • a few services have between 20 and 50, (about 20 services)

    • the vast majority have less than 20 distinct exceptions (300+ services)

@kuhe kuhe marked this pull request as ready for review February 1, 2024 18:23
@kuhe kuhe requested review from a team as code owners February 1, 2024 18:23
@kuhe kuhe requested a review from milesziemer February 1, 2024 18:23
@kuhe
Copy link
Contributor Author

kuhe commented Feb 1, 2024

it is further possible to convert the syntax of the dispatcher from

switch (code) {
  case 'A':
  case 'B':
      throw await de_ExceptionRes(output, context);
  case 'C':
  case 'D':
      throw await de_Exception2Res(output, context);
  default:
    throwDefaultError(...)
}

to

const exceptions = {
  A: 1, // look up the index of the deser fn
  B: 1,
  C: 2,
  D: 2,
};
const deser = [
  de_ExceptionRes,
  de_Exception2Res
];
if (exceptions[code]) {
  throw await deser[exceptions[code]](output, context);
}
throwDefaultError(...)

But this is a very incremental reduction. Does the Smithy team want to have this part included?

Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

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

LGTM.

Clarifying question: as an edge case, is there a possibility of a tree-shaked artifact to become larger with this change? E.g., only importing 1 command, which pre-PR would only import a smaller command-specific error deserializer, compared to post-PR which will now import a larger unified error deserializer?

@syall
Copy link
Contributor

syall commented Feb 5, 2024

it is further possible to convert the syntax of the dispatcher from

switch (code) {
  case 'A':
  case 'B':
      throw await de_ExceptionRes(output, context);
  case 'C':
  case 'D':
      throw await de_Exception2Res(output, context);
  default:
    throwDefaultError(...)
}

to

const exceptions = {
  A: 1, // look up the index of the deser fn
  B: 1,
  C: 2,
  D: 2,
};
const deser = [
  de_ExceptionRes,
  de_Exception2Res
];
if (exceptions[code]) {
  throw await deser[exceptions[code]](output, context);
}
throwDefaultError(...)

But this is a very incremental reduction. Does the Smithy team want to have this part included?

I think we can consider further reduction in a separate PR.

@kuhe kuhe merged commit 958ffaa into smithy-lang:main Feb 5, 2024
6 of 7 checks passed
@kuhe kuhe deleted the feat/error-serde branch February 5, 2024 16:48
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.

None yet

2 participants