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

Reuse buffer for decompression #1247

Closed
wants to merge 2 commits into from
Closed

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Mar 5, 2020

This commit uses a slice pool to store buffers that can be reused for decompression.

Fixes #1239


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Use slices and be careful about ownership.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ashish-goswami and @jarifibrahim)


table/table.go, line 80 at r1 (raw file):

var slicePool = sync.Pool{
	New: func() interface{} {
		b := make([]byte, 5<<10)

Use y.Slice.


table/table.go, line 636 at r1 (raw file):

		return data, nil
	case options.Snappy:
		slicePool.Put(&data)

Use defer or something for this. Can't give up ownership before usage.

@jarifibrahim
Copy link
Contributor Author

The code in this PR won't work. The []byte slice we're inserting into the sync pool is read from the mmaped sst file and any changes to the data byte slice leads to modification of the mmapped buffer of the sst.

The issue is similar to the issue seen in the following snippet.

func do(b []byte) {
	copy(b, []byte("d"))
}

func main() {
	b := []byte("foo")
	fmt.Println(string(b)) // foo
	do(b)
	fmt.Println(string(b)) // doo
}

@jarifibrahim jarifibrahim deleted the ibrahim/decompession-reuse branch March 27, 2020 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Decompression uses too much memory
2 participants