[go] runtime: support runtime.free in noscan size-specialized mallocs
t hepudds (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 3 comments![]( "Open in Gerrit")
if valgrindenabled {
valgrindMalloc(x, size)
}
I think this is technically incorrect, since we don't do a valgrindFree in freeSized. maybe we should?
if gcBlackenEnabled != 0 && elemsize != 0 {
if assistG := getg().m.curg; assistG != nil {
assistG.gcAssistBytes -= int64(elemsize - size)
}
}I think this part might be incorrect. we don't want to subtract anything in this path.
if debug.malloc {
postMallocgcDebug(x, elemsize, typ)
}this is another "maybe we should do this" in the regular code path too. I think it's also fine to just drop from here.
this code is probably automatically added by the generator, so the generator may need some tweaks.
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 2 comments![]( "Open in Gerrit")
if gcBlackenEnabled != 0 && elemsize != 0 {
if assistG := getg().m.curg; assistG != nil {
assistG.gcAssistBytes -= int64(elemsize - size)
}
}I think this part might be incorrect. we don't want to subtract anything in this path.
just kidding, this is correct.
if debug.malloc {
postMallocgcDebug(x, elemsize, typ)
}this is another "maybe we should do this" in the regular code path too. I think it's also fine to just drop from here.
this code is probably automatically added by the generator, so the generator may need some tweaks.
just kidding, this is also correct.
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
Related details
- Michael Matloob
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
if valgrindenabled {
valgrindMalloc(x, size)
}
I think this is technically incorrect, since we don't do a valgrindFree in freeSized. maybe we should?
This is now more explicitly guarded by `runtimeFreegcEnabled`, and `runtimeFreegcEnabled` is now defined in malloc.go to be false when `valgrindenabled` is true, so hopefully that `valgrindMalloc` call never happens.
That said, looking over CL 674077 (which added valgrind support) and grepping the source code, it's not immediately obvious to me how valgrind support in Go is typically tested, and I don't immediately see something like a valgrind builder.
My imprecise sense is that valgrind support is still somewhat best effort.
For now, I'll leave this comment as unresolved to see if there is some way to test this under valgrind.
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
if valgrindenabled {
valgrindMalloc(x, size)
}
t hepuddsI think this is technically incorrect, since we don't do a valgrindFree in freeSized. maybe we should?
This is now more explicitly guarded by `runtimeFreegcEnabled`, and `runtimeFreegcEnabled` is now defined in malloc.go to be false when `valgrindenabled` is true, so hopefully that `valgrindMalloc` call never happens.
That said, looking over CL 674077 (which added valgrind support) and grepping the source code, it's not immediately obvious to me how valgrind support in Go is typically tested, and I don't immediately see something like a valgrind builder.
My imprecise sense is that valgrind support is still somewhat best effort.
For now, I'll leave this comment as unresolved to see if there is some way to test this under valgrind.
I took a quick look at the compiler IR for this `mallocgcSmallNoScanSC2` function on Linux with and without `-tags=valgrind`, which is my understanding of how valgrind support is enabled.
When `-tags=valgrind` is _not_ set, I do not see any calls to `valgrindMalloc`.
When `-tags=valgrind` is set, I can see _other_ calls to `valgrindMalloc` in this function but not the call here guarded by `runtimeFreegcEnabled`, so that is hopefully some confirmation this is working as expected -- `runtimeFreegcEnabled` is false when `valgrindenabled` is true.
So I think I am going to mark this comment as resolved.
I also added a TODO in malloc.go saying it would be nice to check the valgrind integration (though I haven't mailed that updated comment yet).
Related details
- Michael Knyszek
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
This CL is also ready for review (though probably also less important than CL 673695 at this point).
t hepudds (Gerrit)
t hepudds added 1 comment![]( "Open in Gerrit")
if valgrindenabled {
valgrindMalloc(x, size)
}
t hepuddsI think this is technically incorrect, since we don't do a valgrindFree in freeSized. maybe we should?
t hepuddsThis is now more explicitly guarded by `runtimeFreegcEnabled`, and `runtimeFreegcEnabled` is now defined in malloc.go to be false when `valgrindenabled` is true, so hopefully that `valgrindMalloc` call never happens.
That said, looking over CL 674077 (which added valgrind support) and grepping the source code, it's not immediately obvious to me how valgrind support in Go is typically tested, and I don't immediately see something like a valgrind builder.
My imprecise sense is that valgrind support is still somewhat best effort.
For now, I'll leave this comment as unresolved to see if there is some way to test this under valgrind.
I took a quick look at the compiler IR for this `mallocgcSmallNoScanSC2` function on Linux with and without `-tags=valgrind`, which is my understanding of how valgrind support is enabled.
When `-tags=valgrind` is _not_ set, I do not see any calls to `valgrindMalloc`.
When `-tags=valgrind` is set, I can see _other_ calls to `valgrindMalloc` in this function but not the call here guarded by `runtimeFreegcEnabled`, so that is hopefully some confirmation this is working as expected -- `runtimeFreegcEnabled` is false when `valgrindenabled` is true.
So I think I am going to mark this comment as resolved.
I also added a TODO in malloc.go saying it would be nice to check the valgrind integration (though I haven't mailed that updated comment yet).
(I guess I should add that I looked at the IR just before escape analysis, which is after the early dead code elimination done by unified IR, which is probably where the code was eliminated based on the const values, though maybe it was something else that eliminated it.)