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

use serde helper function & serde shortcuts #735

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented Apr 13, 2023

  • omit unnecessary return Promise.resolve(...) in async fn.


  • Add UnaryFunctionCall helper, simplifies mapping function references/expressions like (_) => expectString(_) to expectString

  • Add SerdeElision helper. This decides whether a serde function can be omitted.
return de_MyShape(output);

becomes

return _json(output); 

where _json is a helper function that applies recursive default json serde behavior (omit nullish, filter sparse lists). This allows the omission of de_MyShape and any downstream serde functions. This optimization only occurs if the entire shape and its downstream shapes require no special parsing.


  • reduce repetition in the throwDefaultError function. The service base exception is bound ahead of time instead of in each call.

@kuhe kuhe force-pushed the feat/serde branch 6 times, most recently from 18a8d3b to 3aef19d Compare April 13, 2023 04:03
@kuhe
Copy link
Contributor Author

kuhe commented Apr 13, 2023

sample: effect on aws-sdk-js-v3's client-sagemaker

du -b dist-cjs
7,946	        clients/client-sagemaker/dist-cjs/endpoint
66,390	        clients/client-sagemaker/dist-cjs/models
31,651	        clients/client-sagemaker/dist-cjs/waiters
97,400	        clients/client-sagemaker/dist-cjs/pagination
692,491	        clients/client-sagemaker/dist-cjs/commands
1,267,938	clients/client-sagemaker/dist-cjs/protocols
2,379,401	clients/client-sagemaker/dist-cjs
du -b dist-cjs
7,946	       clients/client-sagemaker/dist-cjs/endpoint
66,390	       clients/client-sagemaker/dist-cjs/models
31,651	       clients/client-sagemaker/dist-cjs/waiters
97,400	       clients/client-sagemaker/dist-cjs/pagination
692,491	       clients/client-sagemaker/dist-cjs/commands
698,736	       clients/client-sagemaker/dist-cjs/protocols
1,810,199      clients/client-sagemaker/dist-cjs/

Overall -24%, or -44% in the protocols file, for this particular JSON protocol client.

Other examples:

service measurement before after %
dynamodb (JSON) du -b dist-cjs 505,550 424,286 -16%
lambda (JSON) du -b dist-cjs 640,077 562,649 -12%

The magnitude of the effect is based on how much of the protocol serde is composed of simple objects that don't need parsing. Doubles, floats (NaN & Infinity strings), timestamps and other types with parsing steps cause the optimization to bail.

JSv3 overall dist-cjs byte count effect provided in linked PR.

* Incompatible types refers to types that need special serde mapping
* functions, like timestamps.
*/
private boolean hasIncompatibleTypes(Shape shape, Set<ShapeType> types, int depth) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a selector equivalent for this?

Set<Shape> matches = selector.select(model);
boolean found = !matches.isEmpty();

if (found) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can be return found from here

@kuhe kuhe merged commit 6b624ad into smithy-lang:main Apr 17, 2023
@kuhe kuhe deleted the feat/serde branch April 17, 2023 13:14
* For determining whether a serde function for a shape may be omitted.
*/
public final class SerdeElision {
private static final Map<Model, SerdeElision> INSTANCES = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

This will leak memory, usually better to use an LRU cache with a capped size or something else that the GC can collect.

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