execgen: introduce // execgen:template directive for bools by jordanlewis · Pull Request #49939 · cockroachdb/cockroach

jordanlewis · 2020-06-06T14:53:36Z

Based on #49728.

This PR adds the // execgen:template<arg1, arg2> syntax for
concrete boolean template variables.

For example:

// execgen:inline
// execgen:template<templateParam>
func a (runtimeParam int, templateParam bool) int {
    if templateParam {
        return runtimeParam + 1
    } else {
        return runtimeParam - 1
    }
}

func main() {
    x := a(0, true)
    y := a(0, false)
    fmt.Println(x, y)
}

This code will print 1, -1 when eventually executed, but the
templateParam boolean will be evaluated at code generation time and not
appear in the generated code at all. Two variants of a() will be
inlined into their callsites - one with templateParam true, and one with
templateParam false.

Release note: None

cockroach-teamcity · 2020-06-06T14:53:44Z

This change is

yuzefovich

Reviewed 5 of 5 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rohany)

pkg/sql/colexec/execgen/inline.go, line 110 at r3 (raw file):

			// Replace template conditionals.
			body = monomorphizeTemplate(body, funcInfo, templateArgs)

Should monomorphizeTemplate call replaceTemplateVars itself?

pkg/sql/colexec/execgen/inline.go, line 245 at r3 (raw file):

					mustInline = true
				}
				if matches := templateRe.FindStringSubmatch(dec); matches != nil {

Where is templateRe defined?

rohany · 2020-06-06T17:59:43Z

I'm trying to build this to play around -- does it compile? Some definitions are missing

pkg/sql/colexec/execgen/inline.go:34:37: undefined: funcInfo
pkg/sql/colexec/execgen/inline.go:166:30: undefined: replaceTemplateVars
pkg/sql/colexec/execgen/inline.go:245:19: undefined: templateRe

rohany · 2020-06-06T18:18:09Z

What's an example of a function that we might want to extend this templating for other types on?

jordanlewis · 2020-06-06T18:19:13Z

Ugh I must have missed a file. It compiles and all tests pass. will fix in a bit.

yuzefovich · 2020-06-08T20:06:44Z

Could you please rebase given that first three commits have been merged?

jordanlewis · 2020-06-08T20:14:07Z

Done.

yuzefovich · 2020-06-08T20:15:01Z

Thanks!

yuzefovich

I have some questions and nits, but overall this looks great!

Reviewed 10 of 10 files at r4, 11 of 11 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, 2 of 2 files at r8, 2 of 2 files at r9.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rohany)

pkg/sql/colexec/hashjoiner_tmpl.go, line 41 at r9 (raw file):

		currentID := hj.ht.probeScratch.headID[i]

		var earlyReturn bool

Is there a reason we can't just use return instead of breaks below? I think it definitely deserves a comment if so.

pkg/sql/colexec/hashjoiner_tmpl.go, line 269 at r9 (raw file):

		}
	}

nit: seems like other functions do have an empty line before return nResults.

pkg/sql/colexec/hashtable_tmpl.go, line 520 at r6 (raw file):

// execgen:inline
// execgen:template<useSel>

Hm, this does look nice in general. Maybe originally I said that I'm in favor of introducing some kind of convention to highlight which variables are runtime and which are template just because in my mind I have a clear distinction between the two since I've spent a lot of time in the old framework. Jordan, I see your point that there is an execgen directive right above that says which variables are of template kind. I think I'm actually neutral on this question now.

pkg/sql/colexec/execgen/inline.go, line 35 at r5 (raw file):

	// Expand functions with // execgen:template.
	funcInfos := createTemplateFuncVariants(f)
	replaceTemplateCallsites(f, funcInfos)

nit: maybe s/Callsites/CallSites/g?

pkg/sql/colexec/execgen/template.go, line 119 at r5 (raw file):

								newElse := monomorphizeTemplate(n.Else, info, args)
								switch e := newElse.(type) {
								case *dst.BlockStmt:

Could you expand on why in the "if" case above we can only have BlockStmt and in the "else" case here we can have something else?

pkg/sql/colexec/execgen/template.go, line 223 at r5 (raw file):

			// Delete template params from runtime definition.
			newParamList := make([]*dst.Field, 0, len(n.Type.Params.List)-len(info.templateParams))
			for i, field := range n.Type.Params.List {

This seems very similar to a code snippet above, can we factor out a function and reuse it in both places? If it's not easy due to different types, then never mind.

pkg/sql/colexec/execgen/template.go, line 259 at r5 (raw file):

				},
			})
			// Extract the remaining execgen directives.

nit: I'd put new line before the comment, not after.

pkg/sql/colexec/execgen/template.go, line 263 at r5 (raw file):

			directives := make([]string, 0)
			for _, s := range funcDecs.Start {
				if strings.HasPrefix(s, "// execgen") {

In templateRe, we have \/\/ * meaning that multiple spaces can match, but here we don't have that. Should we add a preprocessing step that would make all execgen directives use exactly one space after //?

pkg/sql/colexec/execgen/template.go, line 269 at r5 (raw file):

			for i := range argsPossibilities {
				args := argsPossibilities[i]

super nit: could define args variable in for loop itself.

pkg/sql/colexec/execgen/template_test.go, line 27 at r5 (raw file):

	}

	res := generateAllTemplateArgs(arg)

Maybe also add that all 8 args are unique?

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @yuzefovich)

pkg/sql/colexec/hashjoiner_tmpl.go, line 41 at r9 (raw file):

Previously, yuzefovich wrote…

Is there a reason we can't just use return instead of breaks below? I think it definitely deserves a comment if so.

Without something special, you can't early-return from an inlined function.

But then I realized Go has gotos. I added another commit that implements early returns in inlined functions with gotos, which removes the requirement for this change, thanks for the suggestion.

pkg/sql/colexec/hashtable_tmpl.go, line 520 at r6 (raw file):

Previously, yuzefovich wrote…

Hm, this does look nice in general. Maybe originally I said that I'm in favor of introducing some kind of convention to highlight which variables are runtime and which are template just because in my mind I have a clear distinction between the two since I've spent a lot of time in the old framework. Jordan, I see your point that there is an execgen directive right above that says which variables are of template kind. I think I'm actually neutral on this question now.

👍

pkg/sql/colexec/execgen/template.go, line 119 at r5 (raw file):

Previously, yuzefovich wrote…

Could you expand on why in the "if" case above we can only have BlockStmt and in the "else" case here we can have something else?

That's how the AST is defined in Go. An If statement always contains a Body (a block stmt) and optionally an Else (a generic stmt). An if /else if / else is actually 2 nested if statements, where the else of the first if contains the second if/else, if that makes sense.

https://astexplorer.net/#/gist/668b53ce1504c5f02ebf56fe778b6717/26076b4bfe01ea86ad0b1b025cdd9a7b0be6fee4

pkg/sql/colexec/execgen/template.go, line 223 at r5 (raw file):

Previously, yuzefovich wrote…

This seems very similar to a code snippet above, can we factor out a function and reuse it in both places? If it's not easy due to different types, then never mind.

Good idea, but yeah, the types are subtly different so it doesn't feel worth it (sigh)

pkg/sql/colexec/execgen/template.go, line 263 at r5 (raw file):

Previously, yuzefovich wrote…

In templateRe, we have \/\/ * meaning that multiple spaces can match, but here we don't have that. Should we add a preprocessing step that would make all execgen directives use exactly one space after //?

Good point, I guess I would prefer to get rid of the thing that templateRe does and require a single space. I did that instead.

pkg/sql/colexec/execgen/template.go, line 269 at r5 (raw file):

Previously, yuzefovich wrote…

super nit: could define args variable in for loop itself.

Done.

pkg/sql/colexec/execgen/template_test.go, line 27 at r5 (raw file):

Previously, yuzefovich wrote…

Maybe also add that all 8 args are unique?

Thanks, I had meant to do this but forgot. This is a lame test :) Fixed

asubiotto

🥳

Reviewed 10 of 10 files at r4, 8 of 11 files at r5, 9 of 9 files at r10, 2 of 2 files at r11, 2 of 2 files at r12.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @yuzefovich)

pkg/sql/colexec/hash_utils_tmpl.go, line 62 at r10 (raw file):

	nKeys int,
	sel []int,
	hasSel bool,

I think I agree that having the variable names in the directive makes it easy to notice that these are template variables. Not sure if that's because we're all used to templates. @helenmhe what do you think?

pkg/sql/colexec/execgen/testdata/inline, line 180 at r11 (raw file):

func a() {
	{
		foo = bar

This isn't valid go, right?

pkg/sql/colexec/execgen/testdata/template, line 14 at r10 (raw file):

func b(t bool) int {
  if t {
    x = 3

This is slightly confusing. I didn't realize you could reference variables in the outer scope. Wasn't this something you wanted to get rid of?

helenmhe-zz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @yuzefovich)

pkg/sql/colexec/hash_utils_tmpl.go, line 62 at r10 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think I agree that having the variable names in the directive makes it easy to notice that these are template variables. Not sure if that's because we're all used to templates. @helenmhe what do you think?

Yeah I think I prefer including them in the directive over just the different case

rohany

I like this approach more, though I feel like templating isn't quite the right word for what's going on here. I can't think of any alternatives though.

rohany · 2020-06-09T14:43:47Z

pkg/sql/colexec/execgen/inline.go

 			}
-			decl := getTemplateFunc(templateFuncMap, callExpr)
-			if decl == nil {
+			funcInfo := getTemplateFunc(inlineFuncMap, callExpr)


should this still be named getTemplateFunc?

rohany · 2020-06-09T14:45:58Z

pkg/sql/colexec/execgen/template.go

+		templateArgs[i] = dst.Clone(call.Args[param.fieldOrdinal]).(dst.Expr)
+	}
+	// Remove template vars from callsite.
+	newArgs := make([]dst.Expr, 0, len(call.Args)-len(info.templateParams))


That's wierd -- I got panics when I copied the args from the original expression instead of cloning it first, and then removing the other args.

rohany · 2020-06-09T14:47:41Z

pkg/sql/colexec/execgen/template.go

+			argsPossibilities := generateAllTemplateArgs(info.templateParams)
+
+			funcDecs := info.decl.Decs
+			cursor.InsertBefore(&dst.GenDecl{


Could you add a comment here that this is replacing the template func base with a constant? Also, I think this is in conflict with a TODO comment you've left in the inliner.

rohany · 2020-06-09T14:49:21Z

pkg/sql/colexec/execgen/testdata/template

@@ -0,0 +1,142 @@
+inline
+package main


I think it would be useful to have the template test run without the inliner steps

yuzefovich

I feel like templating isn't quite the right word for what's going on here. I can't think of any alternatives though.

Some other ideas are: "preprocessing", "precompilation", "pregeneration", "codegeneration". Still I don't think any of these are better than "templating".

Reviewed 5 of 13 files at r16, 2 of 2 files at r17, 2 of 2 files at r19, 2 of 2 files at r20, 2 of 2 files at r21.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)

pkg/sql/colexec/hashtable_tmpl.go, line 520 at r6 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

👍

One idea is to keep the nice variable names but also introduce a convention that all template variables are at the end of the function signature and require some visual separator (for example, /* runtimeVarsEnd */ before the first template variable when the function signature is on a single line and a separate line with // runtimeVarsEnd when it is on multiple lines).

pkg/sql/colexec/execgen/datadriven_test.go, line 39 at r21 (raw file):

			}
			var sb strings.Builder
			_ = decorator.Fprint(&sb, f)

Is an error ignored here? Maybe we should do t.Fatal(err) if non-nil?

pkg/sql/colexec/execgen/template.go, line 119 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

That's how the AST is defined in Go. An If statement always contains a Body (a block stmt) and optionally an Else (a generic stmt). An if /else if / else is actually 2 nested if statements, where the else of the first if contains the second if/else, if that makes sense.

https://astexplorer.net/#/gist/668b53ce1504c5f02ebf56fe778b6717/26076b4bfe01ea86ad0b1b025cdd9a7b0be6fee4

I see, thanks.

pkg/sql/colexec/execgen/template.go, line 263 at r5 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Good point, I guess I would prefer to get rid of the thing that templateRe does and require a single space. I did that instead.

Just to be clear, by "require a single space" do you mean the writer of the template is responsible for satisfying that requirement?

pkg/sql/colexec/execgen/template_test.go, line 33 at r21 (raw file):

		argsStr := prettyPrintExprs(args...)
		if _, ok := argsMap[argsStr]; ok {
			t.Fatalf("Duplicate template args %s", argsStr)

super nit: we usually use lower case words in the error messages.

Previously, assigning the results of an inlined func with the := syntax produced invalid Go code. Now, it makes sure to define the result variables in the parent scope before entering the new inlined func scope. Release note: None

This commit adds the // execgen:template<arg1, arg2> syntax for concrete boolean template variables. For example: ``` // execgen:inline // execgen:template<templateParam> func a (runtimeParam int, templateParam bool) int { if templateParam { return runtimeParam + 1 } else { return runtimeParam - 1 } } func main() { x := a(0, true) y := a(0, false) fmt.Println(x, y) } ``` This code will print `1, -1` when eventually executed, but the templateParam boolean will be evaluated at code generation time and not appear in the generated code at all. Two variants of `a()` will be inlined into their callsites - one with templateParam true, and one with templateParam false. Release note: None

Early returns in inlined funcs are implemented using gotos. Release note: None

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, @rohany, and @yuzefovich)

pkg/sql/colexec/hashtable_tmpl.go, line 520 at r6 (raw file):

Previously, yuzefovich wrote…

One idea is to keep the nice variable names but also introduce a convention that all template variables are at the end of the function signature and require some visual separator (for example, /* runtimeVarsEnd */ before the first template variable when the function signature is on a single line and a separate line with // runtimeVarsEnd when it is on multiple lines).

Yeah, maybe. I've backed the changes to individual files out of this PR, so we can debate that in a different one.

pkg/sql/colexec/execgen/datadriven_test.go, line 39 at r21 (raw file):

Previously, yuzefovich wrote…

Is an error ignored here? Maybe we should do t.Fatal(err) if non-nil?

This can never fail, since the writer is a string builder.

pkg/sql/colexec/execgen/inline.go, line 61 at r15 (raw file):

Previously, rohany (Rohan Yadav) wrote…

should this still be named getTemplateFunc?

Done.

pkg/sql/colexec/execgen/template.go, line 263 at r5 (raw file):

Previously, yuzefovich wrote…

Just to be clear, by "require a single space" do you mean the writer of the template is responsible for satisfying that requirement?

Yes.

pkg/sql/colexec/execgen/template.go, line 242 at r15 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Could you add a comment here that this is replacing the template func base with a constant? Also, I think this is in conflict with a TODO comment you've left in the inliner.

Done.

pkg/sql/colexec/execgen/template_test.go, line 33 at r21 (raw file):

Previously, yuzefovich wrote…

super nit: we usually use lower case words in the error messages.

Done.

pkg/sql/colexec/execgen/testdata/inline, line 180 at r11 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

This isn't valid go, right?

Yeah, just a test of the inliner/templater which doesn't check Go code validity.

pkg/sql/colexec/execgen/testdata/template, line 14 at r10 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

This is slightly confusing. I didn't realize you could reference variables in the outer scope. Wasn't this something you wanted to get rid of?

Can't do this easily. These tests don't actually get compiled, it's just to test the templating/inlining - wouldn't matter if this was x, or someVarThatDoesntExist.

pkg/sql/colexec/execgen/testdata/template, line 2 at r15 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I think it would be useful to have the template test run without the inliner steps

Done.

jordanlewis · 2020-06-09T23:19:23Z

FYI, I moved all the individual template file changes to #50030.

yuzefovich

Reviewed 20 of 20 files at r22, 9 of 9 files at r23, 2 of 2 files at r24.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @jordanlewis, and @rohany)

jordanlewis · 2020-06-10T00:57:03Z

TFTRs!

jordanlewis · 2020-06-10T00:57:06Z

bors r+

craig · 2020-06-10T02:57:29Z

Build succeeded

GitHub CI (Cockroach)

jordanlewis requested a review from rohany June 6, 2020 14:53

jordanlewis requested a review from a team June 6, 2020 14:53

yuzefovich reviewed Jun 6, 2020

View reviewed changes

jordanlewis force-pushed the execgen-template branch 2 times, most recently from 1e7adc2 to c394e78 Compare June 8, 2020 04:53

jordanlewis changed the title ~~WIP: support templating concrete boolean arguments in execgen~~ execgen: introduce // execgen:template directive for bools Jun 8, 2020

jordanlewis requested a review from a team June 8, 2020 16:38

jordanlewis force-pushed the execgen-template branch from c394e78 to b8bd744 Compare June 8, 2020 20:14

yuzefovich reviewed Jun 8, 2020

View reviewed changes

jordanlewis force-pushed the execgen-template branch from b8bd744 to a541256 Compare June 9, 2020 13:58

jordanlewis commented Jun 9, 2020

View reviewed changes

jordanlewis requested a review from a team June 9, 2020 14:14

asubiotto reviewed Jun 9, 2020

View reviewed changes

jordanlewis requested a review from a team June 9, 2020 14:15

helenmhe-zz reviewed Jun 9, 2020

View reviewed changes

rohany reviewed Jun 9, 2020

View reviewed changes

jordanlewis force-pushed the execgen-template branch from a541256 to 8245af2 Compare June 9, 2020 20:18

yuzefovich reviewed Jun 9, 2020

View reviewed changes

execgen: properly support inlining := calls

2880076

Previously, assigning the results of an inlined func with the := syntax produced invalid Go code. Now, it makes sure to define the result variables in the parent scope before entering the new inlined func scope. Release note: None

jordanlewis force-pushed the execgen-template branch from 8245af2 to b423657 Compare June 9, 2020 22:58

jordanlewis added 2 commits June 9, 2020 19:00

execgen: support early returns in inlined funcs

de6772d

Early returns in inlined funcs are implemented using gotos. Release note: None

jordanlewis force-pushed the execgen-template branch from b423657 to de6772d Compare June 9, 2020 23:00

jordanlewis commented Jun 9, 2020

View reviewed changes

jordanlewis mentioned this pull request Jun 9, 2020

colexec: use new execgen directives on several colexec boolean templates #50030

Open

rohany approved these changes Jun 10, 2020

View reviewed changes

yuzefovich approved these changes Jun 10, 2020

View reviewed changes

craig bot merged commit 3b116e9 into cockroachdb:master Jun 10, 2020

jordanlewis deleted the execgen-template branch June 11, 2020 02:54

Conversation

jordanlewis commented Jun 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 6, 2020

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

rohany commented Jun 6, 2020

Uh oh!

rohany commented Jun 6, 2020

Uh oh!

jordanlewis commented Jun 6, 2020

Uh oh!

yuzefovich commented Jun 8, 2020

Uh oh!

jordanlewis commented Jun 8, 2020

Uh oh!

yuzefovich commented Jun 8, 2020

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

asubiotto left a comment

Choose a reason for hiding this comment

Uh oh!

helenmhe-zz left a comment

Choose a reason for hiding this comment

Uh oh!

rohany left a comment

Choose a reason for hiding this comment

Uh oh!

rohany Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

rohany Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

rohany Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

rohany Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis commented Jun 9, 2020

Uh oh!

yuzefovich left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis commented Jun 10, 2020

Uh oh!

jordanlewis commented Jun 10, 2020

Uh oh!

craig bot commented Jun 10, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jordanlewis commented Jun 6, 2020 •

edited

Loading