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

unique: optimize interning of a byte slice #71926

Open
martin-sucha opened this issue Feb 24, 2025 · 6 comments
Open

unique: optimize interning of a byte slice #71926

martin-sucha opened this issue Feb 24, 2025 · 6 comments
Assignees
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

@martin-sucha
Copy link
Contributor

martin-sucha commented Feb 24, 2025

Proposal Details

Currently the following function allocates a new string for the string(data) conversion even when the value already exists in the unique map.

func makeUnique(data []byte) unique.Handle[string] {
	return unique.Make(string(data))
}

The built-in map has an optimization to not allocate a string for map lookups, this optimization would be similar.

It seems that during the discussions in the original unique proposal the assumptions were that the string conversion will not allocate in this case, but such behavior was not documented in the final proposed API.

This proposal is to:

  • Add an optimization to the compiler/unique package so that unique.Make(string(data)) allocates only when the value does not already exist in the unique map.
  • Update the documentation of the unique package to mention this special case.
  • Update https://go.dev/wiki/CompilerOptimizations to include the optimization.

Such optimization would be useful for parsers that handle byte slices.

@gopherbot gopherbot added this to the Proposal milestone Feb 24, 2025
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the LanguageProposal Issues describing a requested change to the Go language specification. label Feb 24, 2025
@mateusz834 mateusz834 added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal LanguageProposal Issues describing a requested change to the Go language specification. labels Feb 24, 2025
@mateusz834 mateusz834 changed the title proposal: unique: optimize interning of a byte slice unique: optimize interning of a byte slice Feb 24, 2025
@mateusz834
Copy link
Member

This does not need to be a proposal.

@mateusz834 mateusz834 modified the milestones: Proposal, Backlog Feb 24, 2025
@mateusz834
Copy link
Member

mateusz834 commented Feb 24, 2025

Also it would be nice if this worked with structs containing string, then net/netip could take use of that:

go/src/net/netip/netip.go

Lines 1061 to 1063 in fba83cd

case n > 16:
*ip = AddrFrom16([16]byte(b[:16])).WithZone(string(b[16:]))
return nil

go/src/net/netip/netip.go

Lines 489 to 502 in fba83cd

// WithZone returns an IP that's the same as ip but with the provided
// zone. If zone is empty, the zone is removed. If ip is an IPv4
// address, WithZone is a no-op and returns ip unchanged.
func (ip Addr) WithZone(zone string) Addr {
if !ip.Is6() {
return ip
}
if zone == "" {
ip.z = z6noz
return ip
}
ip.z = unique.Make(addrDetail{isV6: true, zoneV6: zone})
return ip
}

@martin-sucha
Copy link
Contributor Author

Also it would be nice if this worked with structs containing string

If I understand correctly, in your example the allocation would happen already when calling WithZone() as zone is a string parameter. Couldn't net/ip call unique.Make for the zone too?

zone := unique.Make(string(b[16:]))
*ip = AddrFrom16([16]byte(b[:16])).WithZone(zone.Value())

@ianlancetaylor ianlancetaylor added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 24, 2025
@ianlancetaylor
Copy link
Member

CC @mknyszek

@mknyszek mknyszek self-assigned this Feb 26, 2025
@mknyszek mknyszek added this to the Backlog milestone Feb 26, 2025
@mknyszek
Copy link
Contributor

You can safely do:

func makeUnique(data []byte) unique.Handle[string] {
	return unique.Make(unsafe.String(&data[0], len(data)))
}

We have a test for it, too.

I've been meaning to document that unique.Make clones strings passed to it (provided they're not stored in an interface) if necessary, making this optimization safe. It would be even safer, of course, if there was some help from the compiler. For now, I think this is an OK workaround.

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
Development

No branches or pull requests

6 participants