Repository navigation
runtime: fix native allocation event reporting - #3043
Merged
cpunion merged 4 commits intoNov 12, 2026
Merged
Conversation
Collaborator
Author
|
@fennoai Please review this fix for native allocation-event reporting. The regression tests fail on main and pass on macOS/arm64 and Linux/amd64. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
This PR wires the native BDWGC runtime's MemStats.Mallocs to the Go allocator's existing size-class object counters via a new MemProfileMallocs(), and adds a focused test suite plus doc updates. The change is well-scoped, honestly commented, and the new aggregation is correct and monotonic.
Verified during review:
MemProfileMallocs()is non-allocating (stack-local sum + atomic loads over a fixed 27-bucket array), so the test'sReadMemStats-allocates-0 assertion holds.- The per-allocation hot path (
recordMemProfileAlloc) is untouched;MemProfileMallocsonly reads counters that path already maintains. - Atomic usage is sound:
atomic.Add/atomic.Loadon the!baremetalpath give per-word consistency; the non-atomic baremetal counter is not activated because the baremetal/wasmReadMemStatssources stats from tinygogc, notMemProfileMallocs(). - No new
unsafeusage, no truncation/overflow concerns (all widening casts,uint64sum), and no security-relevant surface (runtime-internal GC accounting, no untrusted input). - Docs/comments accurately describe the new behavior; the historical "not used because of #2923" text was fully removed and no stale references remain.
One item worth addressing (see inline): Frees is left unset in the BDWGC path, which makes the derived HeapObjects a cumulative allocation total rather than a live-object count. This is likely an intentional BDWGC limitation, but it has an undocumented downstream effect on the runtime/metrics object counters and is worth a one-line note.
Non-blocking review.
Contributor
|
This turn is in managed mode, so I'll return my review as Markdown in my final message rather than publishing a comment or PR review myself. The workspace is already checked out at the same head SHA ( Review: native allocation-event reporting (PR #3043)I reviewed the diff across code-quality, performance, security, and documentation-accuracy, and verified the findings against the actual metrics consumers. The change is well-scoped and correct for its stated goal: On your note that the regression tests fail on Verified during review:
Findings[P2]
So after this change:
This yields an internally inconsistent snapshot: |
Collaborator
Author
Contributor
|
The grep for The re-review is complete. All concerns from my prior review are resolved. Since this turn is in managed mode, I'll return the review as Markdown in my final message. Re-review: PR #3043 — commit
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@fennoai Applied the shared CI fix for the repeated Ubuntu archive TLS failures: prioritize the official Azure HTTPS mirror and retain archive/security HTTPS fallback in the existing setup helper. Sources and signing keys are preserved; helper and workflow-contract tests pass, with no new jobs. Please re-review this CI-only update. |
Contributor
|
I have full verification now. The claims check out:
Let me do one final idempotency check — The change is correct and well-verified. Since this is managed mode, I'll return my re-review as Markdown. Re-review: PR #3043 — commit
|
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
Collaborator
There was a problem hiding this comment.
One non-blocking suggestion to strengthen the allocation-accounting regression coverage.
cpunion
force-pushed
the
fix/2923-bdwgc-allocation-counters
branch
from
November 11, 2026 01:32
dfd097d to
af333c3
Compare
Collaborator
Author
|
@fennoai Added the exact native allocation-event regression requested in review, in af333c3, and rebased onto current main. The isolated probe checks four allocation shapes with exactly 1,000 events, fails on the original base, and passes 10 fresh-process runs each for ordinary and full-LTO builds. Please re-review. |
Contributor
|
All previously-approved changes survived the rebase intact. The PR is complete and consistent. I've verified the new commit Re-review: PR #3043 — commit
|
zhouguangyuan0718
approved these changes
Nov 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2923.
Populate native BDWGC
MemStats.Mallocsfrom the existing managed-allocation counters, soAllocsPerRunandB.ReportAllocsdetect escaping allocations. Adds positive/zero-allocation controls without adding allocator work.For 1,000 escaping 1 KiB slices:
BDWGC byte accounting is unchanged;
Frees/HeapObjectsremain unavailable (zero). The metrics shim no longer substitutes cumulative allocations for the unavailable live-object count.Validated on macOS/arm64 and Linux/amd64, including allocation controls, existing zero-allocation checks, native runtime suites and focused full-LTO tests. WASI allocation and metrics tests also pass.
go vet ./...matches the 12 baseline diagnostics.CI: retain official HTTPS mirror fallback in the existing Ubuntu setup helper after repeated archive TLS failures; no new jobs.