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

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

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

Comments

Projects
None yet
6 participants
@josharian
Copy link
Contributor

commented Mar 30, 2017

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

@rsc

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

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

@rsc

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

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 label Nov 15, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

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

@mvdan

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

@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.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 26, 2018

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2018

Ping @randall77, unless you're on vacation.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 2, 2018

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

internal/bytealg: move IndexByte asssembly to the new bytealg package
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

This comment has been minimized.

Copy link

commented Mar 3, 2018

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

internal/bytealg: move equal functions to bytealg
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

This comment has been minimized.

Copy link

commented Mar 3, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Mar 3, 2018

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

internal/bytealg: move Count to bytealg
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

internal/bytealg: move compare functions to bytealg
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

This comment has been minimized.

Copy link

commented Mar 4, 2018

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

@gopherbot gopherbot closed this in ee58ecc Mar 4, 2018

@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.
You can’t perform that action at this time.