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 a blank indentifier instead of an anonymous field for SDKShapeTraits #453

Merged
merged 2 commits into from
Dec 1, 2015

Conversation

bpot
Copy link
Contributor

@bpot bpot commented Nov 25, 2015

As far as I can tell this retains the current behavior with the following advantages:

  1. The zero-width field has no overhead on instances of the struct. I think SDKShapeTraits is costing us up to 8 bytes on each struct instance (depending on packing/padding).
  2. Cheaper to fetch via reflection. Benchmarks below.
  3. More succinct.

One downside is that _ is less descriptive than SDKShapeTraits but since it's an internal detail that doesn't seem like too big of a deal.

As mentioned in #377 (comment)

Example

Old Style

type InvokeAsyncInput struct {
  // The Lambda function name.
  FunctionName *string `location:"uri" locationName:"FunctionName" min:"1" type:"string" required:"true"`

  // JSON that you want to provide to your Lambda function as input.
  InvokeArgs io.ReadSeeker `type:"blob" required:"true"`

  metadataInvokeAsyncInput `json:"-" xml:"-"`
}

type metadataInvokeAsyncInput struct {
  SDKShapeTraits bool `type:"structure" payload:"InvokeArgs"`
}

New Style

type InvokeAsyncInput struct {
  _ struct{} `type:"structure" payload:"InvokeArgs"`

  // The Lambda function name.
  FunctionName *string `location:"uri" locationName:"FunctionName" min:"1" type:"string" required:"true"`

  // JSON that you want to provide to your Lambda function as input.
  InvokeArgs io.ReadSeeker `type:"blob" required:"true"`
}

Protocol Benchmarks (vs master)

benchmark                                                                      old ns/op     new ns/op     delta
BenchmarkEC2QueryBuild_Complex_ec2AuthorizeSecurityGroupEgress-4               48412         48456         +0.09%
BenchmarkEC2QueryBuild_Simple_ec2AttachNetworkInterface-4                      12853         12925         +0.56%
BenchmarkBuildJSON-4                                                           7597          7700          +1.36%
BenchmarkStdlibJSON-4                                                          7176          6914          -3.65%
BenchmarkJSONRPCBuild_Simple_dynamodbPutItem-4                                 16229         12028         -25.89%
BenchmarkJSONUtilBuild_Simple_dynamodbPutItem-4                                15871         11636         -26.68%
BenchmarkEncodingJSONMarshal_Simple_dynamodbPutItem-4                          6447          6393          -0.84%
BenchmarkRESTJSONBuild_Complex_elastictranscoderCreateJobInput-4               272536        213156        -21.79%
BenchmarkRESTBuild_Complex_elastictranscoderCreateJobInput-4                   9023          7757          -14.03%
BenchmarkEncodingJSONMarshal_Complex_elastictranscoderCreateJobInput-4         38727         37823         -2.33%
BenchmarkRESTJSONBuild_Simple_elastictranscoderListJobsByPipeline-4            14645         10682         -27.06%
BenchmarkRESTBuild_Simple_elastictranscoderListJobsByPipeline-4                10505         8934          -14.95%
BenchmarkEncodingJSONMarshal_Simple_elastictranscoderListJobsByPipeline-4      1073          1056          -1.58%
BenchmarkRESTXMLBuild_Complex_cloudfrontCreateDistribution-4                   428704        369790        -13.74%
BenchmarkRESTXMLBuild_Simple_cloudfrontDeleteStreamingDistribution-4           19239         15089         -21.57%
BenchmarkEncodingXMLMarshal_Simple_cloudfrontDeleteStreamingDistribution-4     5547          5447          -1.80%

benchmark                                                                old allocs     new allocs     delta
BenchmarkJSONRPCBuild_Simple_dynamodbPutItem-4                           79             55             -30.38%
BenchmarkJSONUtilBuild_Simple_dynamodbPutItem-4                          77             53             -31.17%
BenchmarkRESTJSONBuild_Complex_elastictranscoderCreateJobInput-4         1131           819            -27.59%
BenchmarkRESTBuild_Complex_elastictranscoderCreateJobInput-4             59             51             -13.56%
BenchmarkRESTJSONBuild_Simple_elastictranscoderListJobsByPipeline-4      80             56             -30.00%
BenchmarkRESTBuild_Simple_elastictranscoderListJobsByPipeline-4          57             49             -14.04%
BenchmarkRESTXMLBuild_Complex_cloudfrontCreateDistribution-4             1832           1584           -13.54%
BenchmarkRESTXMLBuild_Simple_cloudfrontDeleteStreamingDistribution-4     88             64             -27.27%

benchmark                                                                old bytes     new bytes     delta
BenchmarkEC2QueryBuild_Complex_ec2AuthorizeSecurityGroupEgress-4         10922         10920         -0.02%
BenchmarkJSONRPCBuild_Simple_dynamodbPutItem-4                           3873          2624          -32.25%
BenchmarkJSONUtilBuild_Simple_dynamodbPutItem-4                          3809          2560          -32.79%
BenchmarkRESTJSONBuild_Complex_elastictranscoderCreateJobInput-4         41276         25047         -39.32%
BenchmarkRESTBuild_Complex_elastictranscoderCreateJobInput-4             2800          2384          -14.86%
BenchmarkRESTJSONBuild_Simple_elastictranscoderListJobsByPipeline-4      4368          3120          -28.57%
BenchmarkRESTBuild_Simple_elastictranscoderListJobsByPipeline-4          3328          2912          -12.50%
BenchmarkRESTXMLBuild_Complex_cloudfrontCreateDistribution-4             103383        90447         -12.51%
BenchmarkRESTXMLBuild_Simple_cloudfrontDeleteStreamingDistribution-4     9104          7856          -13.71%

@nightlyone
Copy link

Please put _ struct{} to the front instead of to the end of the struct, see golang/go#12884 for the reason.

@bpot
Copy link
Contributor Author

bpot commented Nov 26, 2015

Thanks @nightlyone, I've moved it to the top and verified that it takes no space now.

@jasdel
Copy link
Contributor

jasdel commented Dec 1, 2015

Hi @bpot thanks for getting this PR together. I compared the differences locally and using _ has nearly the same allocs/bytes improvements vs the interface approach. But your method also has a minor improvement on improves on ns/op and is simpler to read I think.

I don't think there are any issues with breaking changes here since metadata was already private so switching it to _ won't cause an issue.

jasdel added a commit that referenced this pull request Dec 1, 2015
Use a blank indentifier instead of an anonymous field for SDKShapeTraits
@jasdel jasdel merged commit cb8f6e2 into aws:master Dec 1, 2015
@jasdel
Copy link
Contributor

jasdel commented Dec 1, 2015

Looks good thanks for your time, idea, and submitting the PR.

JordonPhillips added a commit that referenced this pull request Dec 4, 2015
skotambkar added a commit to skotambkar/aws-sdk-go that referenced this pull request May 20, 2021
…ws#453)

Adds support for EC2Metadata client to use secure tokens provided by the IMDS. Modifies and adds tests to verify the behavior of the EC2Metadata client.

Fixes aws#457
skotambkar pushed a commit to skotambkar/aws-sdk-go that referenced this pull request May 20, 2021
Breaking Change
---
* `service`: Add generated service for wafregional and dynamodbstreams aws#463
  * Updates the wafregional and dynamodbstreams API clients to include all API operations, and types that were previously shared between waf and dynamodb API clients respectively. This update ensures that all API clients include all operations and types needed for that client, and shares no types with another client package.
  * To migrate your applications to use the updated wafregional and dynamodbstreams you'll need to update the package the impacted type is imported from to match the client the type is being used with.
* `aws`: Context has been added to EC2Metadata operations.([aws#461](aws/aws-sdk-go-v2#461))
  * Also updates utilities that directly or indirectly depend on EC2Metadata client. Signer utilities, credential providers now take in context.
* `private/model`: Add utility for validating shape names for structs and enums for the service packages ([aws#471](aws/aws-sdk-go-v2#471))
  * Fixes bug which allowed service package structs, enums to start with non alphabetic character
  * Fixes the incorrect enum types in mediapackage service package, changing enum types __AdTriggersElement, __PeriodTriggersElement to AdTriggersElement, PeriodTriggersElement respectively.
* `aws`: Client, Metadata, and Request structures have been refactored to simplify the usage of resolved endpoints ([aws#473](aws/aws-sdk-go-v2#473))
  * `aws.Client.Endpoint` struct member has been removed, and `aws.Request.Endpoint` struct member has been added of type `aws.Endpoint`
  * `aws.Client.Region` structure member has been removed

Services
---
* Synced the V2 SDK with latest AWS service API definitions.

SDK Features
---
* `aws`: `PartitionID` has been added to `aws.Endpoint` structure, and is used by the endpoint resolver to indicate which AWS partition an endpoint was resolved for ([aws#473](aws/aws-sdk-go-v2#473))
* `aws/endpoints`: Updated resolvers to populate `PartitionID` for a resolved `aws.Endpoint` ([aws#473](aws/aws-sdk-go-v2#473))
* `service/s3`: Add support for Access Point resources
  * Adds support for using Access Point resource with Amazon S3 API operation calls. The Access Point resource are identified by an Amazon Resource Name (ARN).
  * To make operation calls to an S3 Access Point instead of a S3 Bucket, provide the Access Point ARN string as the value of the Bucket parameter. You can create an Access Point for your bucket with the Amazon S3 Control API. The Access Point ARN can be obtained from the S3 Control API. You should avoid building the ARN directly.

SDK Enhancements
---
* `internal/sdkio`: Adds RingBuffer data structure to the sdk [aws#417](aws/aws-sdk-go-v2#417)
  * Adds an implementation of RingBuffer data structure which acts as a revolving buffer of a predefined length. The RingBuffer implements io.ReadWriter interface.
  * Adds unit tests to test the behavior of the ring buffer.
* `aws/ec2metadata`: Adds support for EC2Metadata client to use secure tokens provided by the IMDS ([aws#453](aws/aws-sdk-go-v2#453))
  * Modifies EC2Metadata client to use request context within its operations ([aws#462](aws/aws-sdk-go-v2#462))
  * Reduces the default dialer timeout and response header timeout to help reduce latency for known issues with EC2Metadata client running inside a container
  * Modifies and adds tests to verify the behavior of the EC2Metadata client.
* `service/dynamodb/dynamodbattribute`: Adds clarifying docs on dynamodbattribute.UnixTime ([aws#464](aws/aws-sdk-go-v2#464))
* `example/service/sts/assumeRole`: added sts assume role example ([aws#224](aws/aws-sdk-go-v2#224))
  * Fixes [aws#157](aws/aws-sdk-go-v2#157) by adding an example for Amazon STS assume role to retrieve credentials.

SDK Bugs
---
* `service/dynamodb/dynamodbattribute`: Fixes a panic when decoding into a map with a key string type alias. ([aws#465](aws/aws-sdk-go-v2#465))
  * Fixes [aws#410](aws/aws-sdk-go-v2#410),  by adding support for keys that are string aliases.
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.

3 participants