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: map lookup with result of inlined method cannot use fast path #44898

rogpeppe opened this issue Mar 9, 2021 · 6 comments
NeedsDecision Performance


Copy link

@rogpeppe rogpeppe commented Mar 9, 2021

$ go version
go version devel +142a76530c Tue Mar 9 22:00:50 2021 +0000 linux/amd64
type Bytes []byte

func (e Bytes) S() string {
	return string(e)

Given the above type definition, I would expect a map lookup of x.S() to compile to the same code as a map lookup of string(x), but it does not.

That is, the following lookup1 function is slower than lookup2, which it should not be:

func lookup1(m map[string]struct{}, name Bytes) bool {
	_, ok := m[name.S()]
	return ok

func lookup2(m map[string]struct{}, name Bytes) bool {
	_, ok := m[string(name)]
	return ok

Here's the code in a benchmark:

Here are the benchmark results:

direct conversion:  6883083	       174.4 ns/op     1024 B/op	       1 allocs/op
conversion in method call: 27336484	        44.18 ns/op        0 B/op	       0 allocs/op
@mvdan mvdan changed the title cmd/gc: map lookup with result of inlined method cannot use fast path cmd/compile: map lookup with result of inlined method cannot use fast path Mar 10, 2021
@ALTree ALTree added this to the Unplanned milestone Mar 10, 2021
Copy link

@networkimprov networkimprov commented Mar 11, 2021

cc @josharian

@toothrot toothrot added the NeedsInvestigation label Mar 11, 2021
Copy link

@toothrot toothrot commented Mar 11, 2021

/cc @randall77

Copy link

@randall77 randall77 commented Mar 11, 2021

I think this is because it literally needs m[string(b)] in the source code to trigger. After inlining, it looks more like:

tmp1 := name
tmp2 := string(tmp1)
_, ok := m[tmp2]

Since this optimization is done in walk, it's hard to determine (1) that the index used is a byte slice converted to a string, and (2) that the byte slice was not modified between the string conversion and the map indexing. (2) is particularly hard at this phase of the compiler.

So while I think this may be fixable, it will certainly take a lot of work. Either plumbing new information into walk, or moving this optimization to SSA where that info is easier to glean. Not sure it's worth all that work.

@randall77 randall77 removed the NeedsInvestigation label Mar 11, 2021
Copy link

@dmitshur dmitshur commented Mar 12, 2021

@randall77 Thanks for investigating this. What state do you think this issue should be moved to next?

@randall77 randall77 added the NeedsDecision label Mar 12, 2021
Copy link
Contributor Author

@rogpeppe rogpeppe commented Mar 13, 2021

FWIW this was quite surprising for me when I came across the issue (the actual code in question was returning the string value of a field in a by-value struct, defined that way to prevent unintentional conversions between to []byte; as it happens, the field was exported, but I had intended to unexport it, which would mean that this using an explicit type conversion inline in the map access wouldn't be possible).

My assumption that the inlined function would be exactly equivalent to the expression inline led to a significant performance regression.

ISTM that doing the optimisation at SSA level would be nicer (and potentially open the door to other optimisations, such as passing []byte to string-argument functions without copying) but maybe that would be too much work?

Copy link

@quasilyte quasilyte commented Jan 28, 2022

Not entirely related, but a careful implementation of #29095 can help here too.
In the referenced issue, we need to inject the constant literals in the inlined functions to make const string optimizations work after the inlining.
Posting here just to create a cross-issue link.

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

No branches or pull requests

8 participants