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

sizeof: new package with constants for Int, Uint, Uintptr, Int8, Float64, etc #29982

Open
robpike opened this issue Jan 29, 2019 · 25 comments
Open

sizeof: new package with constants for Int, Uint, Uintptr, Int8, Float64, etc #29982

robpike opened this issue Jan 29, 2019 · 25 comments

Comments

@robpike
Copy link
Contributor

@robpike robpike commented Jan 29, 2019

See the tail of the (closed) proposal #5602. There I suggested that instead of making unsafe.Sizeof a safe thing, which it still kinda isn't in general, we solve 90% of the problem by add easy-to-use constants to a safe package, such as:

package reflect

const (
  SizeofInt = <size on this machine>
  SizeofUint = SizeofInt
  SizeofBool = 1
  SizeofFloat64 = 8
  //...
)

These would be defined for primitive types only, not slices, maps, etc. You'd still need the unsafe package to handle them.

@gopherbot gopherbot added this to the Proposal milestone Jan 29, 2019
@gopherbot gopherbot added the Proposal label Jan 29, 2019
@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jan 30, 2019

The closest thing we have right now is bits.UintSize, which would be equivalent to the proposed reflect.SizeofUint: https://golang.org/pkg/math/bits/#pkg-constants

Have you considered reversing the names, like IntSize and UintSize? They'd be a bit shorter and easier to remember, I think.

I also wonder if we could remove bits.UintSize in the future, if all these constants are available in the reflect package.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

I think I'm generally -1 on this proposal. I can't see the sizes of any numeric types other than int, uint and uintptr changing in the future. For example, is it remotely possible that uint64 ever gets represented with something other than 8 bytes? I suspect not.

Instead I'd suggest adding one constant to math/bits:

// UintPtrSize is the size of a uintptr in bits.
const UintPtrSize = uintPtrSize

everything else is OK without constants AFAICS.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Jan 30, 2019

we solve 90% of the problem by add easy-to-use constants to a safe package

I'd be interested to hear more about what the problem actually is. What's the use case for knowing representation sizes (as opposed to numeric bit sizes) unless it's to go along with other unsafe operations?

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 30, 2019

@rogpeppe it would be weird to add UintptrSize and not have any routines involving uintptrs in math/bits. (Also it's Uintptr because the lowercase is not uintPtr.)

Adding new constants to reflect does not seem nearly as nice or general as the current unsafe.Sizeof. If we can't find a better home for that language-level constant mechanism, probably its best to just leave it where it is and not have two.

@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Mar 27, 2019

@robpike have your thoughts on this issue changed at all?

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented Mar 27, 2019

I still think it's wrong that the library says that to find the size of int is unsafe: calling unsafe.Sizeof(int) just isn't unsafe. You can compute its size, but it's really tricky and hard to work out from first principles (see the constants blog post for the mechanism).

I still think a handful of constants in someplace safe is a friendlier solution than using unsafe.Sizeof for this subset of its use.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 24, 2019

we solve 90% of the problem by add easy-to-use constants to a safe package, such as:

package reflect

const (
  SizeofInt = <size on this machine>
  SizeofUint = SizeofInt
  SizeofBool = 1
  SizeofFloat64 = 8
  //...
)

These would be defined for primitive types only, not slices, maps, etc.

Nearly all the primitive types are already sized (uint8, float64 etc). The only unsized types are int, uint, uintptr, and bool, right? I am skeptical that there is so much code doing unsafe.Sizeof(true) to find out how big a bool is. Checking the size of an int is certainly common. bits.UintSize is the size of an int or uint in bits (but its bits).

Maybe we could add to reflect just:

const (
    IntSize = ...
    PtrSize = ...
)

Maybe IntSize belongs in math?
Math is mostly about floats, but it does have MaxInt8 etc.

Maybe PtrSize belongs in runtime?
It would be unfortunate for code to import reflect just to find out how big an int is (not as unfortunate as importing unsafe but still unfortunate).

@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented May 14, 2019

@robpike what do you think?

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented May 14, 2019

Sounds fine. I'd be fine with them all in reflect - it's a reflective operation to ask a question about a type, and reflect is part of everything that formats values so it's not a burden to the binary - but am open to other options. Maybe even a standalone tiny package of constants.

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented May 14, 2019

They could even go into builtin...

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Aug 1, 2019

I'm not sure if the ship has sailed on the package to which these belong, but doesn't it seem inconsistent to already have the bit size for a builtin in math/bits (uint), and still declare the bit sizes for int and uintptr in separate packages? I see math/bits as the singular location containing operations and constants to deal with integral types at the bit-level. Declaring these in separate packages would only serve to confuse users and add to the number of dependencies.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 6, 2019

Summary of discussion so far:

I think from the original proposal we're down to IntSize, UintSize, and PtrSize.

For location, math/bits doesn't seem perfect but bits.UintSize already exists.
We could put them elsewhere but no other package seems better.
We don't want to import reflect just for sizes.
Using runtime (next to runtime.GOARCH) would be defensible but maybe not better than math/bits.
Math would be a bit odd since there is nothing about unsized integers in its API today.
And 'builtin' is not a real package at the moment.
Maybe math/bits is the best place.

bits.UintSize already exists.
Do we need to add bits.IntSize too?

bits has nothing to do with pointers, so bits.PtrSize would be a mistake.
Probably reflect.PtrSize is the answer there? Reflect is all about pointers.

So it looks like the open question are:

  1. Do we add bits.IntSize = UintSize ?
  2. Do we add reflect.PtrSize = ... ?
@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented Aug 6, 2019

If you put UintSize into bits, I'd put PtrSize there too so they're in one place.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 20, 2019

@robpike, sorry for the delayed response. I am worried about the message putting PtrSize into math/bits sends. Math/bits is entirely arithmetic operations on integers, hardly any of which are appropriate or even safe to do on Go pointers (maybe TrailingZeros). It would stick out in the API docs ("what is this doing here?"). In contrast, reflect has lots of pointer stuff.

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented Aug 21, 2019

@rsc, I think a comment can cover that well enough, and it sends a nice message to put all the Size constants in one place.

@smasher164

This comment has been minimized.

Copy link
Member

@smasher164 smasher164 commented Nov 25, 2019

Just to clarify, is the current consensus to change the constant declarations in math/bits to something like the following?

const uintSize = 32 << (^uint(0) >> 32 & 1) // 32 or 64

const ptrSize = 32 << (^uintptr(0) >> 63)

const (
    // UintSize is the size of a uint in bits.
    UintSize = uintSize

    // IntSize is the size of an int in bits.
    IntSize = UintSize

    // PtrSize is the size of a uintptr in bits.
    PtrSize = ptrSize
)
@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

@smasher164, I think the consensus is to add IntSize = UintSize to math/bits.

I still have serious reservations about adding PtrSize anywhere but reflect.
There is not a mention of a pointer anywhere in math/bits and I still believe it's important not to even give a hint that bit tricks on pointer representations is a good idea, which the name bits.PtrSize would.

Looking through the conversation history I am not sure why we started talking about the number of bits in a pointer at all.

@rogpeppe, you suggested adding UintptrSize to math/bits.
What was the motivation behind that suggestion?
When would you use it?

Ian points out that UintptrSize == UintSize now on all supported architectures but of course we might not want to assume that for all time. But still, why care about UintptrSize at all?

@rsc rsc moved this from Incoming to Active in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

Ping @rogpeppe or anyone else for why we need UintptrSize at all.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Dec 5, 2019

@rsc For consistency. Why should one numeric type with implementation-defined size be different from the others?

@robpike

This comment has been minimized.

Copy link
Contributor Author

@robpike robpike commented Dec 5, 2019

I will reiterate a point I made above: Much of the argument here is a consequence of the decision to put this in math/bits, where I don't think it belongs. Why not create a new package, called say size, and put them there? This also opens a place to put helpful functions to safely compute things like the size of an array or struct or slice or possibly? map.

@zephyrtronium

This comment has been minimized.

Copy link

@zephyrtronium zephyrtronium commented Dec 14, 2019

I have used the size of uintptr in a set type, effectively map[*T]struct{} but with a Reset method to quickly reuse the same memory. Keys were uintptr, coming from e.g. reflect.ValueOf(x).Pointer(). I used a scrambler of the form A*key+C to try to spread out which bits were used for nearby keys; the values of A and C depend on the size of uintptr. I found after pushing that my original implementation didn't compile specifically on 32-bit targets, because the A and C constants overloaded uintptr there; the final implementation uses apparently nonsense type conversions between uint64 and uintptr on a branch that is dead in the cases that require them to be there.

Later, after much more reading, I got the feeling that garbage collection could move objects while I was using such a set, so I made the keys be just a new field on T; the only requirement for them to remain uintptr was that I had already published the set type with that API.

The overall experience suggests to me that anyone who wants to use the size of uintptr probably should know how to find it (although I'd probably say the same is true of bits.UintSize in the first place, so that isn't a real argument). Also, at least in my case, it was easier to do if ^uintptr(0) != 0xffffffff than to care about the exact size.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 8, 2020

Talked to @robpike about this. Given the trouble with finding an existing home for these, he suggests a new package:

// Sizeof defines the sizes of basic Go types in bytes.
package sizeof

const (
    Bool = 1
    Int8 = 1
    Uint8 = 1
    ...
    Int = ...
    Uint = ...
    Uintptr = ...
)

These could almost be defined as, for example, Int = unsafe.Sizeof(int(0)), but that would result in Int being a uintptr, not an untyped constant.

Having a separate package eliminates all the problems with none of the existing packages being right.

Retitling for this new idea.

@rsc rsc changed the title proposal: reflect: add constants for size of int, bool, etc. proposal: sizeof: new package with constants for Int, Uint, Uintptr, Int8, Float64, etc Jan 8, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 15, 2020

Does anyone have any objections to this new package?
The idea is that it would be import "sizeof".

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 22, 2020

Based on the reactions above, it sounds like we've converged to a likely accept for the new sizeof package.

@rsc rsc moved this from Active to Likely Accept in Proposals Jan 22, 2020
@rsc rsc modified the milestones: Proposal, Go1.15 Jan 22, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 5, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Feb 5, 2020
@rsc rsc changed the title proposal: sizeof: new package with constants for Int, Uint, Uintptr, Int8, Float64, etc sizeof: new package with constants for Int, Uint, Uintptr, Int8, Float64, etc Feb 5, 2020
@rsc rsc modified the milestones: Proposal, Go1.15 Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.