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

Efficient addArray method #388

Closed
wants to merge 2 commits into from
Closed

Efficient addArray method #388

wants to merge 2 commits into from

Conversation

tomijaga
Copy link

@tomijaga tomijaga commented Jun 15, 2022

An efficient solution for the addArray() method addressed in this comment
#368 (comment)

The previous solution used the add() method, which could resize the array more than once.
This PR solves this problem by only resizing the buffer once.

Add Doc comments to fromArray function
@ghost
Copy link

ghost commented Jun 15, 2022

Dear @tomijaga,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA1.

If you decide to agree with it, please visit this issue and read the instructions there.

— The DFINITY Foundation

Footnotes

  1. Contributor License Agreement

@tomijaga tomijaga changed the title Efficient addArray and fromArray buffer methods Efficient addArray method Jun 17, 2022
@matthewhammer
Copy link
Contributor

matthewhammer commented Jun 17, 2022

Thanks for this PR!

Seems very reasonable, except the API is getting large and presenting lots of choices to the programmer now.

The situation addressed here is adding an array to a buffer. They could either use this proposed functionality, or use what is there now in two steps: (1) create a second buffer from their array (via newly merged Buffer.fromArray) and then (2) add entire buffer that via the existing Buffer.append.

The upside of this two-step approach is that it does not require growing the API of the Buffer class, which will enlarge the representation of every buffer instance.

The downside is that the two-step approach will not avoid the extra resizing, which is the whole point of this PR.

But to address that, we could try to absorb some of this clever logic here (Buffer.fromArray could be constant time, with some extra work, and Buffer.append does not do anything clever now, but it could do something like this PR is doing).

WDYT?

@tomijaga
Copy link
Author

tomijaga commented Jun 20, 2022

I think it would be a great idea to add the functionality from this PR to the Buffer.append method. It will improve the buffer without increasing its API.

I'm curious, however, about how Buffer.fromArray would be implemented at constant time. The method that comes to mind is to pass the array as a parameter in the constructor, but that would create a breaking change. It would be interesting to see how it's implemented.

Regarding the update for the Buffer.append method, would you like me to create a new PR with the new logic?
@matthewhammer

@ggreif ggreif force-pushed the master branch 2 times, most recently from d52aecd to 08507fc Compare October 21, 2022 12:22
@tomijaga tomijaga closed this by deleting the head repository Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants