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

Allow decompose to make use of the buffer #132

Merged

Conversation

Gilthoniel
Copy link
Contributor

@Gilthoniel Gilthoniel commented Feb 12, 2024

The comment of Decompose specifies that buf can be used to fill the coefficient if the size is big enough however the current implementation simply ignores it. This is problematic when attempting to encode without allocation. This patch attempts to provide the feature.

@Gilthoniel
Copy link
Contributor Author

@petermattis maybe ?

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Hi @Gilthoniel, thanks for the contribution! Out of curiosity, is this an API that you are using or intend to use?

decomposer.go Show resolved Hide resolved
decomposer.go Show resolved Hide resolved
decomposer.go Outdated Show resolved Hide resolved
decomposer.go Outdated
coefficient = d.Coeff.Bytes()

sizeInBytes := (d.Coeff.BitLen() + 8 - 1) / 8 // math.Ceil(d.Coeff.BitLen()/8.0)
if cap(buf)-len(buf) >= sizeInBytes {
Copy link
Member

Choose a reason for hiding this comment

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

cap(buf)-len(buf)

What does it mean for len(buf) != 0? Should we preserve the data in the slice and only use the unused capacity? The contract here, which comes from golang/go#30870, is ambiguous.

The usual convention for when a function like this takes a scratch space buffer is that it is permitted to overwrite any data in the buffer. So the function checks the capacity (if cap(buf) >= sizeInBytes), sets the length to the needed length if there's room (buf = buf[:sizeInBytes]), and then writes into the buffer from the beginning (coefficient = d.Coeff.FillBytes(buf)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I will change to this approach instead.

Copy link
Member

Choose a reason for hiding this comment

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

Since you made that change, I don't understand why the buf = append(buf, make([]byte, sizeInBytes-len(buf))...) logic is needed, instead of just buf = buf[:sizeInBytes].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right that's not necessary. I actually learned today that it's allowed to increase the length if the capacity is big enough without append.

decomposer.go Outdated Show resolved Hide resolved
@Gilthoniel
Copy link
Contributor Author

@nvanbenschoten thanks a lot for the review, I think I resolved your points. To answer your question, I'm already using the interface to marshal the decimal for transfers over the network. This is much more efficient than a string encoding and reduce the load on the garbage collector, even more with this PR.

@Gilthoniel
Copy link
Contributor Author

@nvanbenschoten ping

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @Gilthoniel.

@nvanbenschoten nvanbenschoten merged commit 1ebf545 into cockroachdb:master Jun 10, 2024
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.

2 participants