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

sync: save 4 bytes memory allocation in WaitGroup struct #19149

Closed
go101 opened this issue Feb 17, 2017 · 8 comments

Comments

@go101
Copy link

@go101 go101 commented Feb 17, 2017

The WaitGroup struct uses a field, state1, a byte array which length is 12 to assure WaitGroup.state() returns a 64-bit aligned pointer. There are 4 bytes wasted by using this trick.
It may be a good idea to use the 4 wasted bytes for the sema field.

Now:

type WaitGroup struct {
	noCopy noCopy
	state1 [12]byte
	sema   uint32
}

func (wg *WaitGroup) state() *uint64 {
	if uintptr(unsafe.Pointer(&wg.state1))%8 == 0 {
		return (*uint64)(unsafe.Pointer(&wg.state1))
	} else {
		return (*uint64)(unsafe.Pointer(&wg.state1[4]))
	}
}

the saving memory version:

type WaitGroup struct {
	noCopy noCopy
	state1 [12]byte
}

func (wg *WaitGroup) state() *uint64 {
	if uintptr(unsafe.Pointer(&wg.state1))%8 == 0 {
		return (*uint64)(unsafe.Pointer(&wg.state1))
	} else {
		return (*uint64)(unsafe.Pointer(&wg.state1[4]))
	}
}

func (wg *WaitGroup) sema() *uint32 {
	if uintptr(unsafe.Pointer(&wg.state1))%8 == 0 {
		return (*uint32)(unsafe.Pointer(&wg.state1[8]))
	} else {
		return (*uint32)(unsafe.Pointer(&wg.state1))
	}
}

Maybe, merging the two functions as one two-returns function is better.

@bradfitz bradfitz changed the title save 4 bytes memory allocation in sync.WaitGroup struct sync: save 4 bytes memory allocation in WaitGroup struct Feb 17, 2017
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 17, 2017

/cc @dvyukov

@bradfitz bradfitz added this to the Go1.9Maybe milestone Feb 17, 2017
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 17, 2017

See #19057, which would make this idea (which is a good one) unnecessary.

@minux

This comment has been minimized.

Copy link
Member

@minux minux commented Feb 17, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Feb 17, 2017

@minux, what if the sync.WaitGroup were embedded in a larger struct?

@go101

This comment has been minimized.

Copy link
Author

@go101 go101 commented Apr 16, 2017

btw, the trick used in WaitGroup here does indicate that compilers should guarantee the alignment of 64-bit words must be 4N. But I can't find the guarantee in Go specification. Other compilers may not enforce the 4N alignment guarantee. So this trick may make Go programs not portable.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 29, 2017

The trick here is not valid. Something has to make the waitgroup 4-byte aligned, and that something is the uint32 field.

@rsc rsc closed this Nov 29, 2017
@rsc rsc reopened this Nov 29, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 29, 2017

Actually, it would be fine (and clearer) to change the [12]byte into [3]uint32. Then the finding of the sema field doesn't even need to use unsafe (except to determine the alignment).

@rsc rsc modified the milestones: Go1.10, Unplanned Nov 29, 2017
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Mar 13, 2018

Change https://golang.org/cl/100515 mentions this issue: sync: make WaitGroup more space-efficient

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.