execgen: add generic inliner #49728

jordanlewis · 2020-05-30T00:17:28Z

This commit adds a new directive to execgen:

// execgen:inline

This directive causes a function that's annotated with it to be inlined
into all of its callers via AST transformation. This kind of inlining is
smarter than before - it permits arguments with names different from the
names of the function's formal parameters, for example.

This commit also changes the functions in distinct_tmpl to use this
directive instead of the manual templating method.

I think this is the only easy function to change. The rest have, at the
simple level of difficulty, static template-time parameters, and at a
harder level of difficulty, type parameters. Improving these cases
holistically should come next.

Release note: None

cockroach-teamcity · 2020-05-30T00:17:36Z

This change is

This was making it hard to run the same benchmarks in a consistent way. Release note: None

rohany

Pretty slick -- do do you have a snippet of what the generated callsites look like now?

pkg/sql/colexec/execgen/cmd/execgen/inline.go

jordanlewis · 2020-06-05T21:42:48Z

https://gist.github.com/jordanlewis/36b2cf775198886c6a65ac91de1911d0

jordanlewis · 2020-06-06T01:56:12Z

Alright, redid a lot of this - added plentiful comments, moved files into pkg/sql/colexec/execgen instead of cmd/execgen, added unit tests for a couple of functions, and added a datadriven test that works on Go files with execgen annotations.

jordanlewis

test

rohany

Thanks for all the comments, it helped alot! A few questions:

This is a decent amount of code, and it helps us inline only this simple case as you mentioned in slack -- what is missing here, and how much code do you forsee needing to add for the more complicated cases?
Will introduction of trying to template stuff affect this, or will it just be another pass (probably scheduled before this one)
I'm worried about execgen becoming more of a "arcane" piece of code than it is now, with the introduction of the dst package -- it's like a mini DSL for go ast manipulation. Maybe that is an unfounded worry though.

pkg/sql/colexec/execgen/inline.go

pkg/sql/colexec/execgen/inline_test.go

pkg/sql/colexec/execgen/testdata/inline

jordanlewis · 2020-06-06T03:29:46Z

Thanks for the review!

This is a decent amount of code, and it helps us inline only this simple case as you mentioned in slack -- what is missing here, and how much code do you forsee needing to add for the more complicated cases?

Yeah, I'm definitely cognizant of how much effort this is. I guess I'm still kind of in an exploratory mode with this work. I'm not completely sure what the end goal is yet.

The next step I'd like to do is templating simple boolean arguments, which seems to be a common thing throughout the exec codebase. For example:

func a(i int) {
   if col.MaybeHasNulls() {
     x = stuff(i, true)
   } else {
     x = stuff(i, false)
   }
}

// execgen:inline
// execgen:template<hasNulls>
func stuff(runtimeArg int, hasNulls bool) int {
   if hasNulls {
      return 3
   } else {
      return 4
   }
}

Currently, this is done with a mix of text/template and regex which although probably less code, is pretty confusing, hacky, and hard to get started with.

Once that's done, the next step would be permitting "mid-stack inlining", so that you can pass template boolean arguments through more than one call site - this is also a common ask. This part seems annoying - you have to keep a graph of functions that are calling each other. It's definitely dawning on me that this is why you have IRs, but I don't think I want to get into messing with any non-AST IRs - if it comes to that I think this project will have jumped the shark.

Once that's done, the next step would be permitting non-boolean template arguments: specifically, types.T template arguments. If that part can be figured out, we'll be able to delete 90% of the custom, per-file template code.

The downside of the code that I'm writing is that inevitably it'll have some subtleties and restrictions - like I'm not planning to try to deal with having conditional expressions that mix template and runtime parameters, the engine will probably just ignore those - so that might be confusing too. I just think it'll be less confusing on the whole, and easier to get started with.

Will introduction of trying to template stuff affect this, or will it just be another pass (probably scheduled before this one)

It'll be a concurrent pass. As we're collecting the functions, we'll also note their template arguments down. As we're inlining the functions, we'll evaluate the template-time conditionals to only include the matched code in the inlined output.

I'm worried about execgen becoming more of a "arcane" piece of code than it is now, with the introduction of the dst package -- it's like a mini DSL for go ast manipulation. Maybe that is an unfounded worry though.

It's definitely a founded worry. But try to take a look at some of the other code we already have. It's very, very arcane, and you have to be arcane in subtle different ways for every operator you write, right now. I'd much prefer to have a single arcane execgen than 100 arcane templates. Also, I hear your point about this becoming arcane, but hopefully the datadriven test styles I've added will make it pretty easy to test and understand.

jordanlewis · 2020-06-06T03:47:10Z

This one guy has some prior art: https://github.com/cosmos72/gomacro/blob/master/doc/generics-c++.md

rohany · 2020-06-06T05:36:27Z

It's definitely dawning on me that this is why you have IRs, but I don't think I want to get into messing with any non-AST IRs - if it comes to that I think this project will have jumped the shark.

Yeah, I'm unsure how far you can get without fancier IR's, but you can also just mock out some IR's with maps of AST nodes to semantic info.

It'll be a concurrent pass. As we're collecting the functions, we'll also note their template arguments down. As we're inlining the functions, we'll evaluate the template-time conditionals to only include the matched code in the inlined output.

Are you sure? It feels to me like inlining and templates are different topics. The templating pass would create the templated versions of these functions, and mark them as inlineable, and then the inline pass goes and inlines them. Im imagining generating two stuff implementations (from above) with inline annotations, replacing each call with the correct one.

Once that's done, the next step would be permitting "mid-stack inlining", so that you can pass template boolean arguments through more than one call site - this is also a common ask. This part seems annoying - you have to keep a graph of functions that are calling each other.

I feel like this will be simpler if inlining and templating are considered separately, but I don't have concrete explanation why.

I'd much prefer to have a single arcane execgen than 100 arcane templates.

Heavily agree, I find the templates difficult to navigate.

yuzefovich

Thanks for pushing forward with this work!

How is the speed of generating code using this new approach in comparison to the old one?

you have IRs

What is "IRs"?

Reviewed 1 of 1 files at r1, 5 of 7 files at r2, 6 of 8 files at r3, 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rohany)

pkg/sql/colexec/main_test.go, line 71 at r1 (raw file):

		flag.Parse()
		if f := flag.Lookup("test.bench"); f == nil || f.Value.String() == "" {

I was wondering about how one would do this at some point but ended up setting an environment variable to false on my machine. Turns out it was this simple, very nice change, thanks!

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

				}
				// Replace the return with the new assignments.
				return &dst.BlockStmt{List: returnAssignmentSpecs}

I wonder why it is a block stmt, does it have to be so? Just curious. Or it doesn't matter because we're using only body.List below?

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

// extractReturnValues generates return value variables. It will produce one
// statement per return return value of the input FuncDecl. For example, for

nit: s/return return/return/.

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

		}
	}
	retVal := &dst.DeclStmt{

nit: s/retVal/retValDeclStmt/.

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

	reassignmentSpecs := make([]dst.Spec, 0, len(formalParams))
	for i, formalParam := range formalParams {
		if inputIdent, ok := callExpr.Args[i].(*dst.Ident); ok && inputIdent.Name == formalParam.Names[0].Name {

Just curious why we have Names[0], can an AST node have multiple names somehow?

pkg/sql/colexec/execgen/inline_test.go, line 72 at r4 (raw file):

			decl: "func foo(a int) (int, string) {}",
			expectedRetDecls: `var (
		__retval_0 int

super nit: I'd indent these expectedRetDecls a little bit to the right, if that's ok.

pkg/sql/colexec/execgen/cmd/execgen/main.go, line 134 at r4 (raw file):

			return err
		}

nit: seems like unnecessary newline.

pkg/sql/colexec/execgen/testdata/inline, line 1 at r3 (raw file):

Previously, rohany (Rohan Yadav) wrote…

cool test!

+1

pkg/sql/colexec/execgen/testdata/inline, line 21 at r4 (raw file):


func a() {
	{

nit: the indentation here seems to be done with tabs, can we adjust that? I don't have a personal opinion on tabs vs spaces, but I think in our codebase we use spaces.

The .eg.go files had unnecessary imports of execgen, which is not required in the runtime generated code, and in fact is suspicious. Release note: None

jordanlewis · 2020-06-06T19:44:51Z

What is "IRs"?

Intermediate representation - the AST is the first representation of a program, but compilers typically transform the AST into several internal representations (like CockroachDB's Expr, opt, distsql trees) for different reasons.

jordanlewis

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

pkg/sql/colexec/main_test.go, line 71 at r1 (raw file):