sstable: generate streamlined blockIter code

[RaduBerinde]

sstable: separate blockIter code

sstable: generate streamlined blockIter code

We automatically generate a "streamlined" version of the blockIter code (in
block_iter_streamlined.gen.go). The streamlined code can be used in the
common case and does not support all features. This improves performance by
reducing the conditionals in the hot path.

The streamlinedBlockIter constant can be used in if statements; in the
streamlined version, it gets replaced by "true" (allowing the compiler to
omit blocks of code from the streamlined version).

[cockroach-teamcity]

This change is 

[RaduBerinde]

sstable/block_iter_streamlined.gen.go line 19 at r2 (raw file):

func (i *blockIter) streamlinedReadEntry() {
	ptr := unsafe.Pointer(uintptr(i.ptr) + uintptr(i.offset))

Comments directly inside the method are missing. I couldn't quite figure out how to fix that, but it doesn't really matter (everyone should always work with the original code).

[petermattis]

Looks straightforward and understandable. Other storage folks should definitely give this a review, but I'm thumbs-up on an approach like this.

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @RaduBerinde)

---

sstable/block-iter-codegen/main.go line 5 at r4 (raw file):

// the LICENSE file.

package main

It's worthwhile to add a comment explaining what this code is doing. It is relatively straightforward, but I still suspect our future selves will thank us. Especially if this code grows more complex over time.

---

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

)

var methods = map[string]struct{}{

Rather than maintaining a list of methods like this, could you automatically deduce the methods by walking over the function declarations and marking any method which references the streamlinedBlockIter constant and any method which transitively calls such a method as needing a rewrite? I'm hoping you'll get the same list (though you might want to exclude init), but then we don't have the possibility of introducing another streamlinedBlockIter path and forget to update this list.

---

sstable/block-iter-codegen/main.go line 46 at r4 (raw file):

	}

	// Rename streamlinedBlockIter ot streamlinedBlockIterTrue.

Is this comment still valid? Seems like you're doing more than what the comment states.

Nit: s/ot/to/g

[RaduBerinde]

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @petermattis)

---

sstable/block-iter-codegen/main.go line 5 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It's worthwhile to add a comment explaining what this code is doing. It is relatively straightforward, but I still suspect our future selves will thank us. Especially if this code grows more complex over time.

Agreed, it's just an initial draft

---

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Rather than maintaining a list of methods like this, could you automatically deduce the methods by walking over the function declarations and marking any method which references the streamlinedBlockIter constant and any method which transitively calls such a method as needing a rewrite? I'm hoping you'll get the same list (though you might want to exclude init), but then we don't have the possibility of introducing another streamlinedBlockIter path and forget to update this list.

Interesting. I think I'd need some more advanced walking infrastructure than Inspect. Might not be worth the trouble, but I'll give it a shot

[petermattis]

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @RaduBerinde)

---

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

Previously, RaduBerinde wrote…

Interesting. I think I'd need some more advanced walking infrastructure than Inspect. Might not be worth the trouble, but I'll give it a shot

Yeah, my suggestion may not be worth the effort. Perhaps just leave a TODO comment in case this becomes some sort of headache in the future.

[RaduBerinde]

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Yeah, my suggestion may not be worth the effort. Perhaps just leave a TODO comment in case this becomes some sort of headache in the future.

I looked at it briefly and I think it's not bad at all, when I get it working I'll update and we can decide then.

[RaduBerinde]

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @petermattis)

---

sstable/block-iter-codegen/main.go line 5 at r4 (raw file):

Previously, RaduBerinde wrote…

Agreed, it's just an initial draft

Done.

---

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

Previously, RaduBerinde wrote…

I looked at it briefly and I think it's not bad at all, when I get it working I'll update and we can decide then.

Done.

[petermattis]

This all looks good to me, but let's get some other Storage 👀 on this PR.

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @RaduBerinde)

---

sstable/block_iter_streamlined.gen.go line 19 at r2 (raw file):

Previously, RaduBerinde wrote…

Comments directly inside the method are missing. I couldn't quite figure out how to fix that, but it doesn't really matter (everyone should always work with the original code).

Hmm, I thought that parser.ParseComments flag would have handled keeping the comments around. Perhaps there is a similar flag that needs to be specified when outputting the AST. I see there is a printer.CommentedNode type. I agree that this isn't worth worrying about.

---

sstable/block-iter-codegen/main.go line 18 at r4 (raw file):

Previously, RaduBerinde wrote…

Done.

Nice!

---

sstable/block-iter-codegen/main.go line 10 at r5 (raw file):

//
// The block_iter.go code uses a special constant streamlinedBlockIter which is
// always false in th code. This program generates streamlined versions of the

Nit: s/th/the/g

[RaduBerinde]

TFTR! I will rope in some more reviewers once I wire it up and maybe demonstrate some gains by inlining the crdb comparer.

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @petermattis)

---

sstable/block_iter_streamlined.gen.go line 19 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Hmm, I thought that parser.ParseComments flag would have handled keeping the comments around. Perhaps there is a similar flag that needs to be specified when outputting the AST. I see there is a printer.CommentedNode type. I agree that this isn't worth worrying about.

Yeah, me too. It does keep most comments, just not the ones that are at the top level inside the body. Apparently they are in separate nodes or something like that.

[RaduBerinde]

sstable/block_iter_streamlined.gen.go line 19 at r2 (raw file):

Previously, RaduBerinde wrote…

Yeah, me too. It does keep most comments, just not the ones that are at the top level inside the body. Apparently they are in separate nodes or something like that.

Yeah, the missing ones all show up under ast.File.Comments