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

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

Open
2 tasks
kedartal opened this issue Jul 7, 2024 · 2 comments
Open
2 tasks
Labels
feature-request A feature should be added or improved. p3 This is a minor priority issue

Comments

@kedartal
Copy link
Contributor

kedartal commented Jul 7, 2024

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

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@kedartal kedartal added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 7, 2024
@jmklix
Copy link
Member

jmklix commented Jul 8, 2024

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.

@jmklix jmklix added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Jul 8, 2024
@kedartal
Copy link
Contributor Author

kedartal commented Jul 8, 2024

@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...

@jmklix jmklix removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

2 participants