Conversation
|
Hi @barryw , Thanks for raising that and for sharing more of the amazing and very powerful infrastructure you have built in such a short time. It is clearly powerful as it has found multiple issues with ease, so whether or not it is useful is IMHO not a question. In terms of the concrete approach taken - there may be alternatives out there when it comes to static code analysis, runtime monitoring, etc. However, an easy, readily available solution that is part of the repo and does not require separate tool installations always wins in my book unless it smells of "reinventing the wheel", which this one does not in my opinion. A few thoughts after looking at this alongside the REST API we already have:
That also follows the precedent of endpoints such as I'd also prefer More generally, I'd like us to avoid gradually accumulating lots of narrowly named endpoints for individual firmware internals. Developer visibility is extremely useful, but ideally the API should still feel cohesive, predictable and extensible a few years from now. I don't think we've reached this point yet. If we feel we are getting close, we may also consider exposing more internals via a dedicated |
Review feedback on GideonZ#776: what the endpoint reports is outstanding allocations grouped by the caller that made them, not FreeRTOS heap blocks. "Blocks" named the allocator's internal structure rather than the thing being measured, which is the sort of implementation detail an API should not leak. GET /v1/machine:heap_allocations PUT /v1/machine:heap_allocations_reset The payload follows the same rename, since it carried the same wrong noun: live_blocks becomes live_allocations, and each caller line reads "N allocations" rather than "N blocks". heap_diff.py parses that line, so it moves with it. The tracker's internal names are untouched. Inside the allocator they really are blocks.
|
@chrisgleissner Thanks — renamed, and I took your naming point further than the URL. You are right that "blocks" named the allocator's internal structure rather than the thing being measured, and the payload carried the same wrong noun, so both moved:
Both configurations rebuilt on the CI-matched Quartus 18.1 nios2 toolchain after the rename: Worth noting the On the flag: agreed, and for the reason you give. On One thing I would put back to you. You wrote that you would avoid two commands for what is really one concept, and cited I have implemented your written version rather than guess at which you meant. The argument for keeping On endpoint sprawl and a This stays a draft either way, pending the design questions in #775 and the |
|
Hi @barryw , great work on all the PRs. One question - where do we stand with the various PRs and especially the draft PRs? Is there anything that you are waiting on from me or Gideon for you to move on with them and get them into a merge-ready state? I see you currently have these PRs open:
It would be good to understand what their dependencies are and what, if anything, blocks them from moving out of draft status, and then from being merged. This also in regards to this PR dependency write-up from above: "This stays a draft either way, pending the design questions in #775 and the $(EXTRA_DEFINES) makefile hook from #763 that did not land with #766." Many thanks |
|
Hi Christian — thanks for asking. The short version is that #789 is first in the chain; #788 and #791 are waiting on it only so their full E2E gates can be rerun green. #776 is independent and is waiting on a design decision rather than another implementation PR. #789 — configuration effectuation lifecycle
#788 — live network service switches
#791 — runtime palette UCI commands
#776 — allocation tracker
So the practical order is:
There is no implementation work I am waiting for you to do. What would help most is review of #789, review of the palette documentation PR, and the two explicit decisions on #776. |
|
Thanks for the overview @barryw . I now asked a question on #789. It is sometimes hard to see what exactly a PR is waiting for, especially if a question was asked during the PR and the discussion then moved on. Thus, to avoid that PRs sit in Draft status indefinitely, I recommend to occasionally check back here and see if any question you are waiting for has been answered, and then just ask it again in a new comment. @GideonZ FYI Barry's reply above shows how all his PR's are related and where he is waiting for your steer. |
|
@chrisgleissner I get emails when my PRs or issues change, so I keep on top of stuff. ;) |
|
@barryw Thanks for moving this and other PRs to merge ready (provided they are merge ready, i.e. all open questions resolved, tested. etc.) and resolving any merge conflicts. Keen to get stuff merged. |
machine:heap says a leak exists. It does not say where, and that has been the
slow part of every leak fixed recently: each one had correct-looking ownership
on every path around it, so reading code did not find them.
This records every live allocation with the address that asked for it, and
reports them grouped by that address. Slots are addressed directly from the
pointer rather than searched, because record and forget sit in the allocator
path and a linear scan there is not affordable once every allocation is
tracked. Four ways per set: a plain direct-mapped table lost a noticeable
number of records to collisions on real runs, and when a set is genuinely full
the oldest entry is dropped and counted, so an under-reporting table says so.
operator new and malloc both funnel through get_mem(), so the tracker
re-attributes each block to the real caller there; without that every
allocation would read as the wrapper.
Nothing is built unless HEAP_TRACK is defined: the entry points compile to
nothing, no table is reserved, and the allocator does no extra work. The
Makefiles gain $(EXTRA_DEFINES) so a diagnostic image is
make u64_no_esp EXTRA_DEFINES=-DHEAP_TRACK=1
The FreeRTOS heap itself takes four guarded lines and no logic.
tests/tools/heap_diff.py is the part that finds leaks rather than merely
listing allocations. A live block is not a leaked block, and on real firmware
most outstanding blocks are legitimate, so it runs the same work twice and
reports which callers grew between rounds. Reading the endpoint once and
treating the large entries as leaks is how you fix the wrong thing; that
mistake nearly attributed the FileInfo leak to the mount cache, because the
totals looked right for it.
Review feedback on GideonZ#776: what the endpoint reports is outstanding allocations grouped by the caller that made them, not FreeRTOS heap blocks. "Blocks" named the allocator's internal structure rather than the thing being measured, which is the sort of implementation detail an API should not leak. GET /v1/machine:heap_allocations PUT /v1/machine:heap_allocations_reset The payload follows the same rename, since it carried the same wrong noun: live_blocks becomes live_allocations, and each caller line reads "N allocations" rather than "N blocks". heap_diff.py parses that line, so it moves with it. The tracker's internal names are untouched. Inside the allocator they really are blocks.
821296b to
934898e
Compare
|
Hi @barryw , the title of this PR says 'Draft: ' but the status of the PR itself is no longer 'Draft'. To avoid ambiguity, I suggest we only use the 'Draft' status of PRs (which clearly flags them as such even in the PR overview) and don't duplicate the word 'Draft: ' in the PR title. Is this PR ready for review and merge? Thanks |
barryw
changed the title
Thanks, @chrisgleissner. Fixed. This is ready, but @GideonZ wants to noodle on it a bit |
2 participants
machine:heap(#766) says a leak exists. This says where. It found four of thefive leaks in #770, #772 and #773, none of which were reachable by reading
code -- each had correct-looking ownership on every path around it.
Shape
software/system/heap_track.{c,h}-- the table and the report. All of itbehind
HEAP_TRACK; with the flag undefined the entry points are macros thatexpand to nothing, no table is reserved and the allocator does no extra work.
heap_4.c-- four guarded lines, no logic. I put the implementation in itsown file specifically so the FreeRTOS vendor diff stays trivial.
memory_wrap.cc-- one call.operator newandmallocboth funnel throughget_mem(), so without re-attributing there, every allocation reads as thatwrapper rather than as the code that asked.
route_machine.cc--GET /v1/machine:heap_allocationsandPUT /v1/machine:heap_allocations_reset, compiled only with the flag.$(EXTRA_DEFINES)appended toOPTIONS(the hook Heap leak detection: a diagnostics endpoint that found a 16 KB-per-call leak #763 proposed,which did not land with Add a machine:heap endpoint for measuring heap leaks #766) plus
heap_track.cin the source list, for thesix actively built targets.
Slots are addressed directly from the pointer rather than searched: record and
forget sit in the allocator path and a linear scan there is not affordable once
every allocation is tracked. Four ways per set, because a plain direct-mapped
table lost a noticeable number of records to collisions on real runs. A full
set drops its oldest entry and counts it in
evicted, so a table that isunder-reporting says so rather than quietly lying.
Gating, demonstrated
The 128 KB table is BSS, so it does not appear in the image either way. Both
configurations build clean on the CI-matched Quartus 18.1 nios2 toolchain.
The part that actually finds leaks
tests/tools/heap_diff.py. A live block is not a leaked block, and on realfirmware most outstanding blocks are legitimate -- the directory on screen, the
current session, cached structures. Reading the endpoint once and treating the
big entries as leaks is how you fix the wrong thing.
So it runs the same work twice and reports which callers grew between rounds.
Steady callers are holding live objects; growing ones are leaking, by the
amount they grow. That is what separated four stranded
FileInfocopies perrun from the seven that were merely the listing on screen in #773 -- and what
stopped me attributing that leak to the disk image mount cache, whose totals
looked right for it and were not.
It goes through
tests/lib/rest.pylike everything else;transport-usagepasses.
Status and what I want to hear
Not yet verified on hardware in this form. The tracker it descends from has
been in daily use all week, but this is a rewrite -- four-way sets, sorted
report, compiled-out gating -- and I have not yet run this code on a device.
Happy to do that before it leaves draft; I did not want to build on a shape
nobody has agreed to.
The open questions are in #775, and the two that matter:
pvPortMalloc/vPortFreeas here. Hooking only
memory_wrap.ccwould touch no FreeRTOS code andwould still have caught every leak found so far, at the cost of missing task
stacks, queues and direct
pvPortMalloccallers.present. If the answer is that it should stay on a branch, that is a fine
answer and nothing is wasted -- it lives on one today.
@chrisgleissner -- this touches no part of the e2e runner, and
heap_diff.pyis standalone, so it should not collide with your multi-device work.