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: opt: eliminate unnecessary slicebytetostring call in argument to mapaccess1_faststr #71132

Closed
adonovan opened this issue Jan 5, 2025 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Jan 5, 2025

In the g function below, a map lookup m[string(byteslice)] avoids allocating a string, presumably because the compiler knows that the use is non-escaping and non-mutating; but if the string conversion is buried within an inlined function, as in the f example below, the optimization is defeated.

type buffer []byte

func f(buf buffer, m map[string]int) int {
	return m[buf.String()] // => slicebytetostring; mapaccess1_faststr
}

func g(buf buffer, m map[string]int) int {
	return m[string(buf.Bytes())] // => mapaccess1_faststr, no allocation
}

func (buf buffer) Bytes() []byte  { return buf }
func (buf buffer) String() string { return string(buf) }

Could the optimization be applied after inlining? It's quite common for the result of a String() method to be used as hash key, and the strings are often constructed from a byte slice (perhaps via a strings.Builder or bytes.Buffer).

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 5, 2025
@prattmic
Copy link
Member

prattmic commented Jan 6, 2025

cc @golang/compiler

@prattmic prattmic added this to the Backlog milestone Jan 6, 2025
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 6, 2025
@prattmic
Copy link
Member

prattmic commented Jan 6, 2025

I believe this optimization is occurring at https://cs.opensource.google/go/go/+/master:src/cmd/compile/internal/walk/order.go;l=337;drc=705b5a569acb9207fda8ea469387d88346b6817a;bpv=1;bpt=1 in walk. walk is after inlining, so that isn't quite the problem.

I wonder if the issue is simply that this is looking for an OBYTES2STR node, but the inlined call would be an OINLCALL node, which itself contains the OBYTES2STR in its body.

@prattmic
Copy link
Member

prattmic commented Jan 6, 2025

Thanks @gabyhelp, this is an exact duplicate of #44898.

@prattmic prattmic closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2025
@thepudds
Copy link
Contributor

thepudds commented Jan 6, 2025

FWIW, Keith mentioned in #44898 (comment) for a similar issue:

I think this is because it literally needs m[string(b)] in the source code to trigger.
[...]
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.

I was wondering if this might be addressable in escape analysis to make it a zero-copy []byte->string conversion , and was taking a quick look at it. (zero-copy string->[]byte conversion exists today in escape analysis, but not the reverse; there are a couple of issues about that).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/640656 mentions this issue: cmd/compile: move []byte->string map key optimization to ssa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

5 participants