Skip to content
This repository has been archived by the owner on Aug 13, 2019. It is now read-only.

refactor: reuse the slice in MemPostings.Delete() #676

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YaoZengzeng
Copy link
Contributor

Signed-off-by: YaoZengzeng yaozengzeng@zju.edu.cn

Signed-off-by: YaoZengzeng <yaozengzeng@zju.edu.cn>
Copy link
Contributor

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

I think I like this.

The

	               found := false
			for _, id := range p.m[n][l] {
				if _, ok := deleted[id]; ok {
					found = true
					break
				}
			}
			if !found {
				p.mtx.Unlock()
				continue
			}

Was a quick path to not allocate new slice. Since we reuse slice we don't need to worry about that.
We can drop this since not matter if found or not we would do full loop over p.m[n][l]. In fact in old code we do it twice.

LGTM from my side, thank you for that!
Wonder if we can do some benchmarks for this to actually see the difference? In theory it should be faster.

@krasi-georgiev
Copy link
Contributor

Automated tsdb bench tests triggered from a comment coming soon(probably next week) so this would a good test for it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants