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 function calls that return a closure #10292

Open
potocnyj opened this Issue Mar 31, 2015 · 6 comments

Comments

Projects
None yet
6 participants
@potocnyj
Contributor

potocnyj commented Mar 31, 2015

https://go-review.googlesource.com/#/c/8200/ added a benchmark to the strings package that shows strings.Trim and its variants allocate memory. The allocation is caused by the call to makeCutsetFunc here. Inlining this call removes the allocation and provides a nice performance boost to Trim. You can see the effect on the benchmark using Go Tip (commit 6262192) below:

benchmark         old ns/op     new ns/op     delta
BenchmarkTrim     3204          2323          -27.50%

benchmark         old allocs     new allocs     delta
BenchmarkTrim     11             0              -100.00%

benchmark         old bytes     new bytes     delta
BenchmarkTrim     352           0             -100.00%

makeCutsetFunc is a pretty simple function (all it does is return a closure), and it was pointed out in review that it might be nice to inline functions like it in the compiler rather than changing strings.Trim explicitly.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Mar 31, 2015

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Mar 31, 2015

Member

In case the link above breaks, the code in question is:

func makeCutsetFunc(cutset string) func(rune) bool {
    return func(r rune) bool { return IndexRune(cutset, r) >= 0 }
}

// Trim returns a slice of the string s with all leading and
// trailing Unicode code points contained in cutset removed.
func Trim(s string, cutset string) string {
    if s == "" || cutset == "" {
        return s
    }
    return TrimFunc(s, makeCutsetFunc(cutset))
}
Member

bradfitz commented Mar 31, 2015

In case the link above breaks, the code in question is:

func makeCutsetFunc(cutset string) func(rune) bool {
    return func(r rune) bool { return IndexRune(cutset, r) >= 0 }
}

// Trim returns a slice of the string s with all leading and
// trailing Unicode code points contained in cutset removed.
func Trim(s string, cutset string) string {
    if s == "" || cutset == "" {
        return s
    }
    return TrimFunc(s, makeCutsetFunc(cutset))
}

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc changed the title from cmd/gc: Inline function calls that return a closure to cmd/gc: inline function calls that return a closure Apr 10, 2015

@rsc rsc changed the title from cmd/gc: inline function calls that return a closure to cmd/compile: inline function calls that return a closure Jun 8, 2015

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 8, 2017

Contributor

I have a WIP for this, which seems to work fine when building a single-package executable but fails as soon as multiple packages are involved since the exporter/importer do not handle OCLOSURE.

@griesemer I'm trying to add support for the OCLOSURE node in the export format but I'm not able to test my changes because I keep getting the following error when I try compile code with: cannot import "runtime/internal/sys" due to version skew - reinstall package (unknown export format version 6 ("version 6"))

How does one test changes to the export format? Is the workflow affected by the recent caching work by @rsc ?

edit: nevermind, I stupidly forgot to update the version check in bimport.go...

Contributor

huguesb commented Nov 8, 2017

I have a WIP for this, which seems to work fine when building a single-package executable but fails as soon as multiple packages are involved since the exporter/importer do not handle OCLOSURE.

@griesemer I'm trying to add support for the OCLOSURE node in the export format but I'm not able to test my changes because I keep getting the following error when I try compile code with: cannot import "runtime/internal/sys" due to version skew - reinstall package (unknown export format version 6 ("version 6"))

How does one test changes to the export format? Is the workflow affected by the recent caching work by @rsc ?

edit: nevermind, I stupidly forgot to update the version check in bimport.go...

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Nov 10, 2017

Contributor
Contributor

davecheney commented Nov 10, 2017

@huguesb

This comment has been minimized.

Show comment
Hide comment
@huguesb

huguesb Nov 10, 2017

Contributor

@davecheney I figured out what was going wrong. Thanks for the offer to help though :)

At this point, my WIP can inline functions with closures that do not capture variables and works fine across packages. I'll upload a CL as soon as I figure out how to properly handle captures (or when I get stuck and need to ask for help).

Interestingly, Trim is no longer a good benchmark for this since the introduction of an ASCII fast path brought a loop into makeCutsetFunc which prevents inlining (this is #14768).

Contributor

huguesb commented Nov 10, 2017

@davecheney I figured out what was going wrong. Thanks for the offer to help though :)

At this point, my WIP can inline functions with closures that do not capture variables and works fine across packages. I'll upload a CL as soon as I figure out how to properly handle captures (or when I get stuck and need to ask for help).

Interestingly, Trim is no longer a good benchmark for this since the introduction of an ASCII fast path brought a loop into makeCutsetFunc which prevents inlining (this is #14768).

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Feb 27, 2018

Change https://golang.org/cl/97415 mentions this issue: cmd/compile: export/import OCLOSURE

gopherbot commented Feb 27, 2018

Change https://golang.org/cl/97415 mentions this issue: cmd/compile: export/import OCLOSURE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment