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

Jump to bottom

cmd/compile: map lookup with result of inlined method cannot use fast path #44898

Open
rogpeppe opened this issue Mar 9, 2021 · 6 comments
Open

cmd/compile: map lookup with result of inlined method cannot use fast path #44898

rogpeppe opened this issue Mar 9, 2021 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Milestone
Unplanned

Comments

@rogpeppe
Copy link
Contributor

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: https://play.golang.org/p/uw-AHzeos-v

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
@networkimprov
Copy link

cc @josharian

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 11, 2021
@toothrot
Copy link
Contributor

/cc @randall77

@randall77
Copy link
Contributor

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 11, 2021
@dmitshur
Copy link
Contributor

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

@randall77 randall77 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 12, 2021
@rogpeppe
Copy link
Contributor Author

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?

@quasilyte
Copy link
Contributor

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
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
8 participants
@toothrot @rogpeppe @networkimprov @dmitshur @mvdan @ALTree @quasilyte @randall77