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: decide what type len and cap params should have in runtime calls #15357

Closed
josharian opened this issue Apr 18, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@josharian
Copy link
Contributor

commented Apr 18, 2016

// TODO: take uintptrs instead of int64s?
func makeslice(t *slicetype, len64, cap64 int64) slice {
func growslice(t *slicetype, old slice, cap int) slice {

We have three candidates, according to the current code: uintptr, int64, and int. It seems to me that they should have type int, which would be an improvement for 32 bit systems. @randall77 ?

@josharian josharian added this to the Unplanned milestone Apr 18, 2016

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

Code is permitted to pass a int64 or uint64 to make, so changing these functions to take type int will require adding code to check for overflow somewhere.

In gccgo I have two different functions, one that takes uintptr and one that takes uint64. The latter function calls the former after checking that the values are in range. The compiler generates a call to the second one if the argument types are larger than uintptr. I can't remember whether I copied this idea from gc or not, but gc doesn't seem to do it today.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2016

Good point. gc doesn't do this today. I'll do some instrumenting and find out how often we'd need to do a double-hop and what the benefit would actually be.

@martisch

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

I would guess the speed improvement is the same for all currently supported 32 bit systems whether it is int or uintptr.

In makeslice and growslice uintptr is already used for e.g. lenmem, capmem, maxcap,... . Also the code already casts (new|old)(len|cap) to uintptr in a few places and newarray also takes a uintptr.

Int would have the benefit that the slice header uses int for cap and len and that are likely to be the most used argument types.

In any case we need to either check cap < 0 for int or cap > maxint for uintptr.

I would be in favor to use uintptr.

@martisch

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Having had a look a bit more int would indeed seem better fitting as josharian suggested. We already check len < 0 in makeslice so passing in a uint/uintptr instead of int that is to large should be caught already. As for uint64/int64 there needs to be a wrapper on 32bit platforms as ianlancetaylor pointed out.
It also seems a better fit with the internals to use int since we changed newarray to take int now (even if it is not used by makeslice anymore).

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2016

Rather than a wrapper, I think we should just add a check to the generated code. Before calling makeslice, on 32 bits systems: (1) check whether the arguments are constants that are too large, in which case fail at compile time, (2) check whether the arguments have int64 type, and if so, insert if /*unlikely*/ int64(int(len)) != len || int64(int(cap)) != cap { panic appropriately }. The relevant code is in walk.go (search for the only instance of makeslice there) if you want to give it a try.

@martisch

This comment has been minimized.

Copy link
Member

commented May 1, 2016

@josharian thanks.
Will try to make a CL for go1.8 that implements this.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2016

Great. Don't hesitate to ask (here or privately over email) if you have questions or want help. The compiler is much easier to work on now that it is in Go and has seen a lot of cleanup, but there's definitely still a learning curve.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 26, 2016

CL https://golang.org/cl/27851 mentions this issue.

@gopherbot gopherbot closed this in e6f9f39 Aug 29, 2016

@golang golang locked and limited conversation to collaborators Aug 29, 2017

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.