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

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

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

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

go101 opened this issue Feb 17, 2017 · 8 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@go101
Copy link

go101 commented Feb 17, 2017 •
edited

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
Copy link
Contributor

/cc @dvyukov

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

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

@minux
Copy link
Member

minux commented Feb 17, 2017 via email

@bradfitz
Copy link
Contributor

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

@go101
Copy link
Author

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.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 15, 2017
@bradfitz bradfitz added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 15, 2017
@rsc
Copy link
Contributor

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 as completed Nov 29, 2017
@rsc rsc reopened this Nov 29, 2017
@rsc
Copy link
Contributor

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
Copy link

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

@go101 go101 mentioned this issue Sep 9, 2018
Closed
@golang golang locked and limited conversation to collaborators Mar 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @rsc @minux @ianlancetaylor @gopherbot @go101