*: force zone.o to link on macOS#15275
*: force zone.o to link on macOS#15275tamird merged 1 commit intocockroachdb:masterfrom tamird:fix-jemalloc-mac-again
Conversation
|
This change is |
|
I wonder why this didn't bite us with c-jemalloc, because Go also builds a static library out of each package, right? Also, I feel obligated to ask: you verified this actually works, yeah? |
pkg/cli/start_jemalloc.go
Outdated
| // #cgo CPPFLAGS: -DJEMALLOC_NO_DEMANGLE | ||
| // #cgo LDFLAGS: -ljemalloc | ||
| // #cgo !darwin LDFLAGS: -ljemalloc | ||
| // #cgo darwin LDFLAGS: -ljemalloc -u_je_zone_register |
There was a problem hiding this comment.
Do you have to do this in all of the cgo directives? I would think this one would be sufficient.
There was a problem hiding this comment.
Yes, one is sufficient, but do you think the inconsistency is worth it?
There was a problem hiding this comment.
Inconsistency doesn't bother me. I would have only made this change to server/status/runtime_jemalloc.go because that is the only one we care about. Also, add a comment explaining what is going on.
There was a problem hiding this comment.
What do you mean? The problem here is not a reporting problem; before this change, jemalloc is not being used in our macOS builds, period.
Added a comment.
There was a problem hiding this comment.
before this change, jemalloc is not being used in our macOS builds, period.
Do we care other than the stats reporting? Btw, given that this is the second time this has broken, perhaps a small test is in order.
There was a problem hiding this comment.
Hm, this behaviour is specific to macOS, which we don't run CI on. How would we test it?
I think we care, even in the absence of stats reporting, but probably just for consistency with other platforms.
There was a problem hiding this comment.
Hm, this behaviour is specific to macOS, which we don't run CI on. How would we test it?
Developers run tests on macOS frequently. A test would have caught this very quickly. Also, seems possible for this to break on another platform as well.
There was a problem hiding this comment.
PS I don't particularly care how many places you add -u_je_zone_register. The test is the more important bit.
tamird
left a comment
tamird
left a comment
There was a problem hiding this comment.
I wonder why this didn't bite us with c-jemalloc, because Go also builds a static library out of each package, right?
It's because cgo mashes all the object files into a single object file called _all.o before creating the static archive; the linker is only able to omit whole files, not single functions, so it is forced to keep everything.
Also, I feel obligated to ask: you verified this actually works, yeah?
Yes.
pkg/cli/start_jemalloc.go
Outdated
| // #cgo CPPFLAGS: -DJEMALLOC_NO_DEMANGLE | ||
| // #cgo LDFLAGS: -ljemalloc | ||
| // #cgo !darwin LDFLAGS: -ljemalloc | ||
| // #cgo darwin LDFLAGS: -ljemalloc -u_je_zone_register |
There was a problem hiding this comment.
Yes, one is sufficient, but do you think the inconsistency is worth it?
| } | ||
|
|
||
| // Used to force allocation in tests. | ||
| func allocateMemory() { |
There was a problem hiding this comment.
Why not put this in the test file?
|
Because `import "C"` is not supported in tests. Can't make this stuff up.
…On Apr 22, 2017 22:11, "Nikhil Benesch" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/server/status/runtime_jemalloc.go
<#15275 (comment)>
:
> @@ -120,3 +126,9 @@ func getJemallocStats(ctx context.Context) (uint, uint, error) {
return uint(js.Allocated), uint(js.Resident), nil
}
+
+// Used to force allocation in tests.
+func allocateMemory() {
Why not put this in the test file?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#15275 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPHVjYf7gYA0YBBSIRRL8KTqkXiZ5ks5ryrNOgaJpZM4NFRN8>
.
|
This is the mechanism by which jemalloc hooks into `malloc` on macOS; without it, jemalloc is not used. Closes #15272.
4 participants
This is the mechanism by which jemalloc hooks into
mallocon macOS;without it, jemalloc is not used.
Closes #15272.