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

Simplifies readFrom and WriteTo, and improves their performance. #142

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

lemire
Copy link
Member

@lemire lemire commented Oct 12, 2023

On one benchmark, this PR doubles the performance. With this PR, we reach 1.7 GB/s (reading and writing to memory). This is for reading and writing, so that the bandwidth is 3.4 GB/s which is not great, but... Note that this for in-memory... if you write and read to a disk... it is going to be slower.

Before...

$ go test -bench  BenchmarkBitsetReadWrite -benchmem
goos: darwin
goarch: arm64
BenchmarkBitsetReadWrite-8         69720             16075 ns/op              16 B/op          2 allocs/op
PASS
ok      _/Users/dlemire/CVS/github/bitset       1.533s

After...

$ go test -bench  BenchmarkBitsetReadWrite -benchmem
goos: darwin
goarch: arm64
BenchmarkBitsetReadWrite-8        151460              7396 ns/op            2064 B/op          4 allocs/op
PASS
ok      _/Users/dlemire/CVS/github/bitset       1.321s

Fixes #141

@lemire
Copy link
Member Author

lemire commented Oct 12, 2023

@thanhpk @klauspost @omerfirmak @king526 : Please review/comment.

I really don't understand why our code is so convoluted when what I wrote is just... obvious? Is there a catch? Why do we have all this complexity in the first place ?

@thanhpk
Copy link
Contributor

thanhpk commented Oct 12, 2023

@lemire you are trading space for time, it would alloc a big chunk of memory, crashing some programs if the bitset is big enough

We already discussed this
#103

@omerfirmak
Copy link
Contributor

omerfirmak commented Oct 12, 2023

@lemire you are trading space for time

-benchmem to see the GC hit you are taking. We, at Nethermind, don't depend on bitset anymore (due to #134). But I would rather keep the GC at minimum.

@lemire
Copy link
Member Author

lemire commented Oct 12, 2023

@thanhpk

it would alloc a big chunk of memory, crashing some programs if the bitset is big enough

I don't think it would. The overhead memory usage should still effectively constant:

func readUint64Array(reader io.Reader, data []uint64) error {
	length := len(data)
	bufferSize := 128
	buffer := make([]byte, bufferSize*int(wordBytes))
	for i := 0; i < length; i += bufferSize {
		end := i + bufferSize
		if end > length {
			end = length
			buffer = buffer[:wordBytes*uint(end-i)]
		}
		chunk := data[i:end]
		if _, err := io.ReadFull(reader, buffer); err != nil {
			return err
		}
		for i := range chunk {
			chunk[i] = uint64(binaryOrder.Uint64(buffer[8*i:]))
		}
	}
	return nil
}

func writeUint64Array(writer io.Writer, data []uint64) error {
	bufferSize := 128
	buffer := make([]byte, bufferSize*int(wordBytes))
	for i := 0; i < len(data); i += bufferSize {
		end := i + bufferSize
		if end > len(data) {
			end = len(data)
			buffer = buffer[:wordBytes*uint(end-i)]
		}
		chunk := data[i:end]
		for i, x := range chunk {
			binaryOrder.PutUint64(buffer[8*i:], x)
		}
		_, err := writer.Write(buffer)
		if err != nil {
			return err
		}
	}
	return nil
}

@omerfirmak

I would rather keep the GC at minimum.

Yeah. It seems that binary.Read and binary.Write are allocating functions. But we can just reuse the buffer, see my code above.

@thanhpk
Copy link
Contributor

thanhpk commented Oct 13, 2023

@lemire I agree

@lemire lemire merged commit 2cc58bd into master Oct 13, 2023
30 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.

ReadFrom Slow!!!
3 participants