Skip to content

core/types, internal/ethapi, signer/core/apitypes: avoid copying 128KB blobs in range loops#33717

Merged
rjl493456442 merged 2 commits intoethereum:masterfrom
adblesss:fix-blob-range-copy
Feb 3, 2026
Merged

core/types, internal/ethapi, signer/core/apitypes: avoid copying 128KB blobs in range loops#33717
rjl493456442 merged 2 commits intoethereum:masterfrom
adblesss:fix-blob-range-copy

Conversation

@adblesss
Copy link
Copy Markdown
Contributor

kzg4844.Blob is 131072 bytes. Using for _, blob := range copies the entire blob on each iteration. With up to 6 blobs per transaction, this wastes ~768KB of memory copies.

Switch to index-based iteration and pass pointers directly.

@adblesss adblesss changed the title types,ethapi,signer: avoid copying 128KB blobs in range loops core/types, internal/ethapi, signer/core/apitypes: avoid copying 128KB blobs in range loops Jan 29, 2026
rjl493456442
rjl493456442 previously approved these changes Jan 30, 2026
@rjl493456442
Copy link
Copy Markdown
Member

Can you demonstrate the allocation difference? My test doesn't show any allocation.

// BenchmarkAllocation-8   	24739767	        48.65 ns/op	       0 B/op	       0 allocs/op
func BenchmarkAllocation(b *testing.B) {
	var slice [][4096]byte
	for i := 0; i < 128; i++ {
		slice = append(slice, [4096]byte{0xa, 0xb})
	}
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		for _, blob := range slice {
			_ = blob
		}
	}
}

@adblesss
Copy link
Copy Markdown
Contributor Author

The compiler optimizes away the copy when blob isn't used. Here's a test that shows the actual difference:

func BenchmarkBlobToCommitmentIndex(b *testing.B) {
    blobs := make([]kzg4844.Blob, 6)
    for i := 0; i < b.N; i++ {
        for j := range blobs {
            kzg4844.BlobToCommitment(&blobs[j])
        }
    }
}

func BenchmarkBlobToCommitmentCopy(b *testing.B) {
    blobs := make([]kzg4844.Blob, 6)
    for i := 0; i < b.N; i++ {
        for _, blob := range blobs {
            kzg4844.BlobToCommitment(&blob)
        }
    }
}

Results:

BenchmarkBlobToCommitmentIndex-10    2.2 ms/op
BenchmarkBlobToCommitmentCopy-10     2.5 ms/op  (~13% slower)

@MariusVanDerWijden
Copy link
Copy Markdown
Member

I don't see much of a difference tbh:

// BenchmarkBlobToCommitmentIndex-14    	       1	2515741619 ns/op	37191880 B/op	  208720 allocs/op
// BenchmarkBlobToCommitmentCopy-14    	       1	2210367940 ns/op	36923936 B/op	  208160 allocs/op

@rjl493456442
Copy link
Copy Markdown
Member

I will on hold this PR if it doesn't show the difference in terms of allocation.

@adblesss
Copy link
Copy Markdown
Contributor Author

adblesss commented Feb 2, 2026

Your benchmark ran only once (N=1) which gives unreliable results due to variance.

With more iterations (-benchtime=5s, N~2500):

BenchmarkBlobToCommitmentIndex-10    2.2 ms/op    918 allocs/op
BenchmarkBlobToCommitmentCopy-10     2.5 ms/op    918 allocs/op  (~12% slower)

The allocation count is the same because the copy happens on the stack, not heap.
The improvement is in CPU time from avoiding the 128KB memcpy.

@rjl493456442
Copy link
Copy Markdown
Member

OK. Can you rebase and resolve the conflicts?

@adblesss
Copy link
Copy Markdown
Contributor Author

adblesss commented Feb 3, 2026

@rjl493456442 resolve conflicts

@adblesss adblesss requested a review from rjl493456442 February 3, 2026 07:25
@rjl493456442 rjl493456442 added this to the 1.17.0 milestone Feb 3, 2026
@rjl493456442 rjl493456442 merged commit 54a91b3 into ethereum:master Feb 3, 2026
7 of 8 checks passed
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.

3 participants