aws / aws-sdk-cpp

AWS SDK for C++
Apache License 2.0
1.96k stars 1.05k forks source link

Expose `ByteBuffer` values by const reference from `DynamoDB::Model::AttributeValue` #3031

Open kedartal opened 2 months ago

kedartal commented 2 months ago

Describe the feature

When fetching from DynamoDB and accessing a binary data field, AttributeValue::GetB() makes a copy of the underlying ByteBuffer (that's held in a shared pointer to hopefully an appropriate AttributeValueValue instance).

/// returns the ByteBuffer if the value is specialized to this type, otherwise an empty Buffer
const Aws::Utils::ByteBuffer GetB() const;

This indeed allows returning an empty buffer if the type is not actually ValueType::BYTEBUFFER, but in the happy path, also probably the very common case, this means we're making a copy of all fetched data. It seems like we could support the same API without such copy.

Use Case

Avoiding the pointless copy should benefit performance (mostly better utilization of memory cache) and reduce memory requirements.

Proposed Solution

If the type is indeed ValueType::BYTEBUFFER, return a const reference to the underlying ByteBuffer, otherwise return a const reference to a static empty ByteBuffer, which could be e.g. a static member of AttributeValueValue.

An even better solution would be to allow moving the underlying data, something like

Aws::Utils::ByteBuffer GetBWithOwnership()

where, in case of a type mismatch, the implementation could simply instantiate an empty buffer.

Other Information

The same holds true for the other DynamoDB value types. Since the API returns a default when the type is not as expected, it may as well return a const reference to the actual data in the happy path, otherwise a const reference to a static default of the appropriate type. Or, even better, allow moving all types of values.

Acknowledgements

jmklix commented 2 months ago

Thanks for opening this feature request. This is something that would be a nice improvement to this sdk, but it is not a high priority for us currently. I don't have a timeline for when this might get added to this sdk.

kedartal commented 2 months ago

@jmklix Thanks. Would you consider just adding an accessor for the shared pointer AttributeValue::m_value? This would allow interested users to circumvent the issue cleanly in their own client code; otherwise their only option is maintaining a patched version of the sdk...