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

runtime, strings, bytes: undo use of runtime as dumping ground for shared code #19792

Closed
josharian opened this issue Mar 30, 2017 · 16 comments
Closed

runtime, strings, bytes: undo use of runtime as dumping ground for shared code #19792

josharian opened this issue Mar 30, 2017 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@josharian
Copy link
Contributor

From CL 38693:

@martisch said:

I think it is not a good general direction that more and more code is shared between string and bytes by moving it into the runtime and that code seems actually not be used by the runtime.
Maybe a separate github discussion would be good about alternatives and moving the shared code out of the runtime and just e.g. importing the bytes package code into the string package

@randall77 replied:

This is a good question. We are starting to dump a lot of stuff in runtime that is really just "stuff that two other packages have to share". Perhaps a new internal package somewhere would be more appropriate.

This issue is to continue/resolve the discussion.
@josharian josharian added this to the Go1.9Maybe milestone Mar 30, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 7, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 7, 2017
@rsc
Copy link
Contributor

rsc commented Jun 26, 2017

It would be good for neither bytes to depend on strings nor strings to depend on bytes. Everything depends on runtime so that's the dumping ground. If we want an internal package that both bytes and strings depend on, that seems OK.

@rsc
Copy link
Contributor

rsc commented Jun 26, 2017

Also, on March 30, "this issue is to continue/resolve the discussion." Should anyone else be in this discussion?

@rsc
Copy link
Contributor

rsc commented Jun 26, 2017

@bradfitz points out that maybe there's no good reason to disallow strings -> bytes or vice versa (obviously we can't have both). So the effect of one or the other should be considered too.

@bradfitz
Copy link
Contributor

@randall77, do you want to make this decision?

I'm fine with either a new internal package, or strings depending on bytes. (if we have to pick a dependency direction, it intuitively makes more sense for strings to depends on bytes, since strings are made of bytes... but that's pretty arbitrary)

@randall77
Copy link
Contributor

Here's the list of assembly functions that are currently in runtime that don't actually implement the runtime API:

bytes·Compare
bytes·countByte
bytes·Equal
bytes·IndexByte
bytes·indexShortStr
bytes·supportsVX
reflect·call
strings·countByte
strings·IndexByte
strings·indexShortStr
strings·supportsVX
sync∕atomic·AddInt32
sync∕atomic·AddInt64
sync∕atomic·AddUint32
sync∕atomic·AddUint64
sync∕atomic·AddUintptr
sync∕atomic·CompareAndSwapInt32
sync∕atomic·CompareAndSwapInt64
sync∕atomic·CompareAndSwapUint32
sync∕atomic·CompareAndSwapUint64
sync∕atomic·CompareAndSwapUintptr
sync∕atomic·LoadInt32
sync∕atomic·LoadInt64
sync∕atomic·LoadPointer
sync∕atomic·LoadUint32
sync∕atomic·LoadUint64
sync∕atomic·LoadUintptr
sync∕atomic·StoreInt32
sync∕atomic·StoreInt64
sync∕atomic·StoreUint32
sync∕atomic·StoreUint64
sync∕atomic·StoreUintptr
sync∕atomic·SwapInt32
sync∕atomic·SwapInt64
sync∕atomic·SwapUint32
sync∕atomic·SwapUint64
sync∕atomic·SwapUintptr
syscall·naclWrite
syscall·now
time·now

I think all the sync/atomic ones are for the race detector. The other non strings/bytes ones are very special cases that probably need to remain in runtime. So I think just solving this problem for strings/bytes is enough.

I'm ok for these functions to live in bytes, and have strings import bytes. I'd really rather have them live in internal/???, but I can't think of a good name for ???. bytealg? Suggestions welcome.

I'll fix this for 1.11. strings->bytes unless someone comes up with a good name.

@randall77 randall77 modified the milestones: Go1.10, Go1.11 Nov 15, 2017
@randall77 randall77 self-assigned this Nov 15, 2017
@randall77 randall77 added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 15, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 15, 2017
@mvdan
Copy link
Member

mvdan commented Nov 16, 2017

Perhaps a combination of runtime or internal with bytes? Like rbytes or runbytes.

@mvdan
Copy link
Member

mvdan commented Nov 16, 2017

Some more suggestions from a friend: data, mem, rawdata, rawmem.

@bradfitz
Copy link
Contributor

@mvdan, there's nothing "runtime" about such a new package, though, so blending in "r" or "run" to the name doesn't make much sense.

internal/bites. The assembly might bite you if you're not careful. Little playful, but also internal so doesn't matter. But probably too playful and hard to discuss verbally.

internal/strbyte?

I'm fine with internal/bytealg too.

I'm also fine with strings depending on bytes.

@randall77, can you pick something and do this earlier in the cycle than later? Some CLs like https://go-review.googlesource.com/c/go/+/87935 depend on it.

@josharian josharian mentioned this issue Feb 14, 2018
Closed
@gopherbot
Copy link

Change https://golang.org/cl/87935 mentions this issue: strings: optimize Count for arm64

@bradfitz
Copy link
Contributor

Ping @randall77, unless you're on vacation.

@randall77
Copy link
Contributor

Just back today. I will get to this this week.

@gopherbot
Copy link

Change https://golang.org/cl/98016 mentions this issue: internal/bytealg: move IndexByte asssembly to the new bytealg package

gopherbot pushed a commit that referenced this issue Mar 2, 2018
@randall77
internal/bytealg: move IndexByte asssembly to the new bytealg package
403ab0f 
Move the IndexByte function from the runtime to a new bytealg package.
The new package will eventually hold all the optimized assembly for
groveling through byte slices and strings. It seems a better home for
this code than randomly keeping it in runtime.

Once this is in, the next step is to move the other functions
(Compare, Equal, ...).

Update #19792

This change seems complicated enough that we might just declare
"not worth it" and abandon.  Opinions welcome.

The core assembly is all unchanged, except minor modifications where
the code reads cpu feature bits.

The wrapper functions have been cleaned up as they are now actually
checked by vet.

Change-Id: I9fa75bee5d85db3a65b3fd3b7997e60367523796
Reviewed-on: https://go-review.googlesource.com/98016
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/98355 mentions this issue: internal/bytealg: move equal functions to bytealg

gopherbot pushed a commit that referenced this issue Mar 3, 2018
@randall77
internal/bytealg: move equal functions to bytealg
1dfa380 
Move bytes.Equal, runtime.memequal, and runtime.memequal_varlen
to the bytealg package.

Update #19792

Change-Id: Ic4175e952936016ea0bda6c7c3dbb33afdc8e4ac
Reviewed-on: https://go-review.googlesource.com/98355
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/98495 mentions this issue: internal/bytealg: move Count to bytealg

@gopherbot
Copy link

Change https://golang.org/cl/98515 mentions this issue: internal/bytealg: move compare functions to bytealg

gopherbot pushed a commit that referenced this issue Mar 4, 2018
@randall77
internal/bytealg: move Count to bytealg
45964e4 
Move bytes.Count and strings.Count to bytealg.

Update #19792

Change-Id: I3e4e14b504a0b71758885bb131e5656e342cf8cb
Reviewed-on: https://go-review.googlesource.com/98495
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Mar 4, 2018
@randall77
internal/bytealg: move compare functions to bytealg
f6332bb 
Move bytes.Compare and runtime·cmpstring to bytealg.

Update #19792

Change-Id: I139e6d7c59686bef7a3017e3dec99eba5fd10447
Reviewed-on: https://go-review.googlesource.com/98515
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/98518 mentions this issue: internal/bytealg: move short string Index implementations into bytealg

@golang golang locked and limited conversation to collaborators Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
6 participants
@bradfitz @josharian @rsc @mvdan @randall77 @gopherbot