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: adjust _HeapAllocChunk dynamically to improve performance #16866

Closed
toddboom opened this issue Aug 24, 2016 · 7 comments

Comments

Projects
None yet
7 participants
@toddboom
Copy link

commented Aug 24, 2016

I know there is a huge desire to limit the number of user-facing variables for memory allocation and garbage collection, but through internal testing @rw found that increasing GO_HEAP_MALLOC_MB can give us performance gains that we can't obtain by modifying GOGC alone. Specifically, increasing it to 64MB from the default of 1MB gave us a ~60% boost in write throughput.

I realize that opening this up for user modification might be a non-starter, but I figured I'd bring it up for discussion since the 1.8 dev cycle is kicking off. Curious to hear what you guys think about this. Let me know if you've got any questions about our use case or testing.

Here's a quick patch for the change that @rw put together:

--- src/runtime/malloc.go       2016-08-12 00:04:10.524693000 +0000
+++ src/runtime/malloc.go       2016-08-12 00:12:04.328693000 +0000
@@ -109,6 +109,27 @@
        _PageMask  = _PageSize - 1
 )

+// modeled after `readgogc`
+func readgoheapallocmb() uintptr {
+       p := gogetenv("GO_HEAP_MALLOC_MB")
+       if p == "" {
+               return 1 << 20
+       }
+       mb := atoi(p)
+       sz := mb << 20
+       println("GO_HEAP_MALLOC_MB", mb)
+       println("GO_HEAP_MALLOC_MB as bytes", sz)
+       return uintptr(sz)
+}
+
+// we're in the allocator guts here, so ensure this is always sane by
+// initializing it:
+var _HeapAllocChunk uintptr = 1 << 20
+
+func init() {
+       _HeapAllocChunk = readgoheapallocmb()
+}
+
 const (
        // _64bit = 1 on 64-bit systems, 0 on 32-bit systems
        _64bit = 1 << (^uintptr(0) >> 63) / 2
@@ -129,7 +150,6 @@

        _FixAllocChunk  = 16 << 10               // Chunk size for FixAlloc
        _MaxMHeapList   = 1 << (20 - _PageShift) // Maximum page length for fixed-size list in MHeap.
-       _HeapAllocChunk = 1 << 20                // Chunk size for heap growth

        // Per-P, per order stack segment cache size.
        _StackCacheSize = 32 * 1024

/cc @pauldix @jwilder @rw

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 24, 2016

You're proposing a solution but barely mentioning the problem.

What is your problem or motivation with this patch?

@aclements

This comment has been minimized.

Copy link
Member

commented Aug 25, 2016

Interesting. @toddboom, can you describe your workload, in particular how it grows/shrinks the heap over time and how much heap it uses? A GODEBUG=gctrace=1 log would be particularly concrete. Even better would be a benchmark that exhibits the improvement with this change.

Rather than adding a knob, I suspect we could just adjust this dynamically. If the heap is large already, allocate more heap in larger chunks.

@aclements

This comment has been minimized.

Copy link
Member

commented Aug 27, 2016

@toddboom, it would also be useful to know more about the hardware where you got the 60% boost. For example, is it a large multicore?

@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2016

Ping @toddboom ?

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

Given the admitted "huge desire to limit the number of user-facing variables for memory allocation and garbage collection", I think it's pretty clear we're not going to make one just for this one setting. We'd love to hear more about the bigger context here, though, and adjust it automatically in the runtime as appropriate. Thanks.

@rsc rsc changed the title proposal: make GO_HEAP_MALLOC_MB configurable by environment variable runtime: adjust _HeapAllocChunk dynamically to improve performance Oct 20, 2016

@toddboom

This comment has been minimized.

Copy link
Author

commented Oct 21, 2016

@rsc Understood. I owe you guys some more detail on this, but I've been swamped on the product side. I'll dig in and try to provide more detail over the next few weeks. Thanks for all the feedback so far!

@rsc rsc modified the milestones: Go1.9Early, Go1.8Maybe Nov 2, 2016

@bradfitz bradfitz removed the WaitingForInfo label Jan 6, 2017

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Early May 3, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jun 3, 2017

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this Jun 3, 2017

@golang golang locked and limited conversation to collaborators Jun 3, 2018

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.