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: utf8.DecodeRune should be fast/slow-path inlineable #31666

Open
josharian opened this issue Apr 24, 2019 · 6 comments
Open

cmd/compile: utf8.DecodeRune should be fast/slow-path inlineable #31666

josharian opened this issue Apr 24, 2019 · 6 comments

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Apr 24, 2019

We manually partially inline utf8.DecodeRune in a few places in the tree. For example, in bytes.EqualFold, we have:

		if s[0] < utf8.RuneSelf {
			sr, s = rune(s[0]), s[1:]
		} else {
			r, size := utf8.DecodeRune(s)
			sr, s = r, s[size:]
		}

It'd be nice to rewrite utf8.DecodeRune like this:

func DecodeRune(p []byte) (r rune, size int) {
	if len(p) > 0 && p[0] < RuneSelf {
		return rune(p[0]), 1
	}
	return decodeRuneSlow(p)
}

and then let mid-stack inlining take it from there. (And delete all the manual partial inlining.)

Unfortunately, this has a cost of 89, over the budget of 80.

This is basically just another variant of #17566, but I'm filing it separately since that issue is super noisy.

cc @bradfitz @mvdan @dr2chase

@martisch
Copy link
Contributor

@martisch martisch commented Apr 25, 2019

While at it we should make it also work for utf8.DecodeRuneInString at the same time.

@mvdan
Copy link
Member

@mvdan mvdan commented May 8, 2019

Perhaps very clear cases like this can be used to start tweaking the inlining heuristic in the right direction, with the smallest steps possible.

@corona10
Copy link
Contributor

@corona10 corona10 commented May 13, 2019

@martisch @mvdan Can I take a look at this issue?

@mvdan
Copy link
Member

@mvdan mvdan commented May 13, 2019

@corona10 you're certainly welcome to take a look, but if you're looking for low-hanging fruit issues to work on, this isn't a good candidate. It likely requires someone with a good understanding of the compiler, and it's not a simple fix, as @josharian already gave that a try.

@corona10
Copy link
Contributor

@corona10 corona10 commented May 13, 2019

@mvdan
Oh.. @josharian already tried it. Looks like a not simple issue.
I thought it was just an issue of rewriting so that it can help the compiler to do a mid stack inlining, but I did not know that cost was an issue to consider. I will try to find some easier issues.

@renthraysk
Copy link

@renthraysk renthraysk commented Jul 14, 2019

binary.Uvarint() has the same problem. Impossible to write the 1 byte fast path mid stack func without exceeding cost.

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
5 participants