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

runtime: adjust _HeapAllocChunk dynamically to improve performance #16866

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

runtime: adjust _HeapAllocChunk dynamically to improve performance #16866

toddboom opened this issue Aug 24, 2016 · 7 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Go1.10

Comments

@toddboom
Copy link

toddboom commented Aug 24, 2016 •
edited

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

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

What is your problem or motivation with this patch?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 25, 2016
@aclements
Copy link
Member

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

@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?

@quentinmit quentinmit added this to the Go1.8Maybe milestone Sep 6, 2016
@aclements aclements mentioned this issue Sep 29, 2016
Closed
@adg
Copy link
Contributor

adg commented Oct 3, 2016

Ping @toddboom ?

@rsc
Copy link
Contributor

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

@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 Issue is not actionable because of missing required information, which needs to be provided. label Jan 6, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Early May 3, 2017
@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 3, 2017
@gopherbot
Copy link

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.)

@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.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
7 participants
@bradfitz @toddboom @rsc @quentinmit @aclements @adg @gopherbot