go-bindings: pass large arrays by ref instead of value #393
Conversation
|
Hey @holiman, I've actually been wanting to do something like this for a while. But I didn't want to break backwards compatibility and compatibility with go-kzg-4844. I would also support passing the other parameters by reference too. In your benchmarks this appears to be noticeably slower though. Am I reading this correctly? |
I see the same, and I'm guessing it's a fluke. Those are the benches which run in c-land on separate threads, right? I guess my laptop was just busy, so corrupted the benchmarks. This one is also funky: |
|
As for breaking backwards compatibility: Option 1: add new methods which takes by reference. Everyone happy. |
I don't believe the Cgo call happens in a different thread, but I'm not entirely sure how Go handles things under the hood. I have run the benchmarks myself and the performance difference is negligible.
Here, Go only tracks allocations in Go-land. It cannot track the C allocations.
Yes, this is the approach we'd take. |
Right, but that is beside the point. The funky was that benchstat completely ignored a tangible improvement :) |
Also make the code a bit more go-idiomatic, by returning nil when there is also an error returned. This commit also changes the error-check, to avoid map-lookups in the happy case. Also the map lookup was converted into a switch clause. The unmarshalling methods were changed to be less memory-intense, by decoding directly into the destination objects.
bindings/go/main.go
Outdated
| return nil | ||
| } | ||
| return err | ||
| return fmt.Errorf("unexpected return value from c-library %v", ret) |
There was a problem hiding this comment.
I removed the panic here. I think libraries should generally avoid panic, especially in cases where they already have a channel to signal error - in this case just returning an error. The the caller can decide if it's panic-worthy or if it can clean up some files or whatever before exiting
|
Thanks for the PR! Two questions:
|
Example: package main
import "fmt"
func foobar(data [10]byte) {
data[0] = 13
fmt.Printf("data is: %x\n", data)
}
func main() {
var data [10]byte
fmt.Printf("data is: %x\n", data)
foobar(data)
fmt.Printf("data is: %x\n", data)
}yields So passing a Anyway: as long as we're in Go, the compiler might be clever. It might see that it the call destination doesn't actually mutate the input array, so it can omit creating a pristine copy of the arguments. It can inline certain functions, thus avoiding the decision alltogether. But if it's going to take an array as input argument (and remember, it must not be mutated!) ,and pass the reference to it to c, it obviously must copy it, because it has no insight into what Sidenote: The same thing does not apply for
More info: https://go.dev/blog/slices-intro And that answers question number 2: So the And yes, I noticed from a profile, when doing |
I just found it weird that this was the only array which was converted to pointers. When looking at the benchmark calls, I thought those parameters were being handled differently but they are not.
This is just a nit, but it matches the order defined in C.
bindings/go/main.go
Outdated
| // will return nil. | ||
| func makeErrorFromRet(ret C.C_KZG_RET) error { | ||
| switch ret { | ||
| case C.C_KZG_OK: |
There was a problem hiding this comment.
This is not expected to happen, all current callsites avoid invoking it for this path
There was a problem hiding this comment.
I agree. Just wanted to maintain the same order as C. I would actually like to remove this case & state that it should only be called if ret isn't C_KZG_RET. How would you feel about this?
There was a problem hiding this comment.
Yeah fine by me as long as it doesn't panix
| if !loaded { | ||
| panic("trusted setup isn't loaded") | ||
| } | ||
| if blob == nil { |
There was a problem hiding this comment.
Why so defensive? If caller sends in nil, he should not be surprised if a nil deref happens, imo
There was a problem hiding this comment.
I would rather be safe than sorry. It's a very simple check, might as well.
jtraglia
left a comment
jtraglia
left a comment
There was a problem hiding this comment.
I think this looks great. Thanks for your work on this @holiman! I pushed a few minor changes, nothing controversial. I would like to wait to merge this PR until @kevaundray has expressed the desire to move forward with this change in go-kzg-4844 too. And after merging this, I think we should make our first v1.0.0 release.
|
Sure no rush to merge, but technically, equivalence between go-kzg and c-kzg is not required, and, also already a lost cause, as go takes more inputs in some cases ( e.g. num-threads), and returns more values in some. |
|
Not sure though, maybe this PR takes the "by ref" too far. Small arrays are typically fine to pass be value, making them alloc-free since they're stack-allocated. In geth, we nearly alway pass common.Hash ( Cc @karalabe ptal at the api changes desceibed above |
Hello again. Any thoughts here? I agree that passing small arrays by value should not be an issue. In this regard, IMO we should optimize for the simplest and most pleasant API both for the library itself and its users (geth). I guess the change implied here would be to only pass Blobs by reference, and pass everything else by value. WFM. |
|
I agree with @asn-d6 |
|
Yeah we all agree it should be pointers for blobs (and any other large structs) but values for smallish things. I'll change it back |
|
Sounds good to me too. Thanks @holiman. |
|
Should be good to go now, IMO |
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
* crypto/kz4844: pass blobs by ref (ethereum#29050) This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63 * update version patch * run go mod tidy * use blob reference --------- Co-authored-by: Martin HS <[email protected]> Co-authored-by: Mason Liang <[email protected]>
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63 (cherry picked from commit d5bacfa)
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
This change makes use of the following underlying changes to the kzg-libraries in order to avoid passing large things on the stack: - c-kzg: ethereum/c-kzg-4844#393 and - go-kzg: https://github.com/crate-crypto/go-kzg-4844/pull/63
5 participants
This PR
Blobby value, to instead pass*Blob, that is , by reference.It also changes the returns to also be mainly pointer-based. In this change, it also returnsnilin case there is an error, which is go-idiomaticNOTE: This means that this PR creates a breaking change, which requires a major version-upgrade of this library, strictly speaking.⚠️
The
Blobtype is anarray, of 130K bytes. As opposed to a slice of similar size, when an array is passed by value, every byte is passed over the heap.This is the old API:
Changed API in this PR:
Untouched:
Corresponding go-kzg PR: https://github.com/crate-crypto/go-kzg-4844/pull/63