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

cmd/compile: inline functions with only an error wrapper #48192

Open
dsnet opened this issue Sep 4, 2021 · 5 comments
Open

cmd/compile: inline functions with only an error wrapper #48192

dsnet opened this issue Sep 4, 2021 · 5 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Sep 4, 2021

In a number of cases, an input argument only escapes through the error value. Examples of this:

In order to avoid escaping the input, one possibility is to copy the input before placing it in an error. This has the unfortunate downside of always allocating in the error case in situations when the input would have already been treated as being escaped.

Another way to work around this is to take advantage of "function outlining", where the part that causes an input to escape is moved to an inlineable wrapper function so that the compiler can better prove whether to allocate depending on how the function is used by the caller.

Example attempt at function outlining in strconv:

func ParseFloat(s string, bitSize int) (float64, error) {
     n, err := parseFloat(s, bitSize)
     if err != nil {
          err = &NumError{"ParseFloat", s, err}
     }
     return n, err
}
func parseFloat(s string, bitSize int) (float64, error)

Unfortunately ParseFloat is not inlineable since it exceeds the inline budget. However, if it were within budget, then there are situations where we can avoid the NumError allocation and treating s as escaping. For example:

n, err := strconv.ParseFloat(s, 64)
ok := err == nil
return n, ok

In this snippet, the caller doesn't even care about the error value other than the fact that it was non-nil. If ParseFloat was inlineable, the compiler can prove that NumError does not escape and avoid heap allocating it. In fact, the compiler can (in theory) go a step further and realize that the error wrapping is dead code and eliminate it entirely (all we cared about was whether it was nil).

It would be nice if functions of the following pattern could be inlinable:

func Foo(...in) (...out, error) {
    out..., err := foo(in...) 
    if err != nil {
        err = ... // wrap the error somehow
    }
    return out..., err
}

I don't know if this means increasing the inline budget or matching on such code patterns.

\cc @bradfitz @josharian @martisch @FiloSottile

@josharian
Copy link
Contributor

@josharian josharian commented Sep 4, 2021

Yes please.

In practice I suspect the easiest way to accomplish immediately this is to do pattern-matching and increase the budget on match.

More broadly speaking it would be nice if we could somehow predict when partial inlining will result in escape analysis changes, and use that as a signal to increase the budget. But I don't know how to do that simply and cheaply.

cc @mdempsky

@dsnet
Copy link
Member Author

@dsnet dsnet commented Sep 4, 2021

will result in escape analysis changes

Not just escape analysis changes, but also significant dead code elimination 😃

@josharian
Copy link
Contributor

@josharian josharian commented Sep 4, 2021

Yep. One question is about making inlining decisions per-function vs per call-site. The latter can get more expensive more quickly, but is more powerful.

It occurs to me that potential escape analysis changes might be available by comparing the escape analysis of the trampoline function and the called function, when they have the same signature. Plausible, @mdempsky? I guess we have a phase ordering problem, but escape analysis is cheap and high impact, so maybe worth doing twice?

The dead code removal has to be be per-callsite. But if we are doing purely local, brittle AST-based heuristics, like "err != nil", maybe that'd be cheap enough.

@josharian
Copy link
Contributor

@josharian josharian commented Sep 4, 2021

cc also @CAFxX for lock inlining

@martisch
Copy link
Contributor

@martisch martisch commented Sep 5, 2021

I think this and other cases like #48195 boil down to the question if we want to add a new inline trigger that gets a function and makes analysis like:

  • escape with and without inlining
  • pattern (e.g. is it just a dispatch function with a switch or an if that only ever triggers one of the cases)
  • cost of code blocks covered by branches
    ...

paired with a more tuned cost model of ast nodes and patterns.

Then we could save some extra information like: If this argument is constant at the calling site then inline, if this output is unused then inline. This e.g. should also trigger for using strings.Index to directly call strings.IndexByte when the Index is a constant string of length 1.

Unless there is some near time direction to support feedback driven optimisation as a first class citizen I would say lets go for it if someone can commit todo the work. We should however invest a bit in making the cost and trigger model easy to configure and verify. e.g. have test similar to asm tests that check some code samples are still inlined as wanted. I would not go so far as having a per ARCH cost model yet but I think the current cost model of ast nodes could be improved to assign lesser costs on patterns of comparisons of if statements.

Another case that I would hope will get optimised as a pattern is e.g.:

func LastIndexByte(s []byte, c byte) int {
	return bytealg.LastIndexByte(s, c)
}

where the function is just an internal forward to another package e.g. arch specific asm function that is selected by build tags. Note that the call might have some extra duplicated or cost arguments.

@cagedmantis cagedmantis added this to the Backlog milestone Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants