On Sun, Sep 06, 2026 at 09:16:08PM +0200, Pavel Sanda wrote:
> I have candidate for a leak when I relax the demand it's 2.5.x only. So it's
> present in 2.4 as well,
> but perhaps the windows Qt6 transition made it more pronounced?
Irrespective of testers, it's probably worth fixing anyway.
One more news - when the band-aid is done around Action leak, a second
perpendicular
issue pops up, but it's not "leak", but fragmentation issue in
row-breaking/metrics
code, where the size of paragraphs matters: 800 chars cost 0.3 MB in 20 short
paragraphs
vs 5.2 MB in one.
Since this is glibc allocator "issue", I have no clue how that translates to
windows.
I have too many things on my plate now to do any proper review, so only
attaching the reports from Claude.
First is the Action finding. Can someone familiar with that part of the code
look at the claim and patch?
Second report is the fragmentation finding. JMarc, that one might be of your
interest.
Pavel
================================================================
Accumulating QActions in DynamicMenuButton::updateTriggered()
================================================================
Summary
-------
Two toolbar dropdowns rebuild their menu on every dispatched user action
and orphan the QAction objects they replace. The orphans stay alive until
the window is closed, so memory grows for as long as a LyX session is
edited. The growth is independent of document size and is not recovered
by closing the document; only restarting LyX clears it.
Affected code: src/frontends/qt/GuiToolbar.cpp,
DynamicMenuButton::updateTriggered(), branches "textstyle-apply" and
"paste" (loadFlexInsets() has the same shape but is called only when the
text class changes, so it is bounded).
Mechanism
---------
The menu is rebuilt like this:
m->clear();
...
Action * act = new Action(func, QIcon(), toqstr(lb), toqstr(s), this);
m->addAction(act);
The last argument of the Action constructor is the QObject parent, and it
is "this", i.e. the DynamicMenuButton, not the menu. QMenu::clear() only
deletes actions that the menu itself owns; actions owned by another object
are merely detached. Consequently every rebuild allocates a fresh set of
Action objects (with their QStrings and icons) and leaves the previous set
attached to the button, where it remains until the button is destroyed.
Trigger frequency
-----------------
GuiToolbar::update() runs on every keypress - the function carries an
in-code comment saying exactly that - and emits updated(), which is
connected to DynamicMenuButton::updateTriggered(). The path is:
GuiApplication::dispatch -> updateCurrentView -> restartCaret
-> updateDialogs -> updateToolbars -> GuiToolbar::update
-> updated() -> DynamicMenuButton::updateTriggered()
This is reached by any dispatched action, not only text input: typing and
plain cursor movement were both measured to accumulate at the same rate.
Measurements
------------
Environment: LyX master (2.6.0dev), Qt 5.15.2, Linux, self-compiled.
Workload driven by real X key events (xdotool) on a private Xvfb, with the
typed text verified afterwards against the status-bar character count.
Metric: VmRSS read immediately after the workload, comparing two binaries
that differ only by the proposed patch.
workload unpatched with patch removed
800 chars, 20 short paragraphs 3.8 MB 0.3 MB 92%
800 chars, one long paragraph 8.3 MB 5.2 MB 38%
2400 chars, one long paragraph 31.5 MB 17.3 MB 45%
Per keystroke with paragraphs of normal length: 4.83 kB unpatched versus
0.36 kB with the patch.
The residual growth visible in the long-paragraph rows is a different,
unrelated effect and is described in the second report.
Proposed fix
------------
Rebuild the menu only when its contents actually changed. The same
function already does this for the flex-inset branch by caching
d->text_class_; the patch caches the free-font list and the clipboard
selections in DynamicMenuButton::Private in the same style and returns
early when they are unchanged. This removes the orphaned actions and also
avoids rebuilding two menus on every keypress.
Patch: dynamicmenubutton_rebuild_on_change.patch
An alternative, cruder variant (deleting the stale actions before
rebuilding) was also measured. It removes roughly three quarters of the
growth but is less complete, because default_action in the "paste" branch
is constructed with "this" as parent and never passed to addAction(), so
it is not among m->actions() and would still be orphaned once per update.
Notes and caveats
-----------------
- Both dynamic menus are present in 2.4.x as well, so this is not a 2.5
regression. A Qt6 build may make each orphaned action more expensive,
which would be consistent with the reports being louder on 2.5/Windows,
but that has not been verified.
- All figures above are Qt5 on Linux. Confirmation on Windows/Qt6 is
still missing.
- Deleting actions while a tear-off menu is open deserves a look; the
proposed patch avoids the question by not rebuilding at all when
nothing changed.
Reproduction
------------
Open any document, type a few hundred characters (or hold a cursor key,
which repeats at roughly 30 actions per second), and watch RSS. Growth
tracks the number of dispatched actions and is unaffected by document
size. Closing the document does not release it; restarting LyX does.
diff --git a/src/frontends/qt/GuiToolbar.cpp b/src/frontends/qt/GuiToolbar.cpp
index 2e0a6d5819..8f368deef5 100644
--- a/src/frontends/qt/GuiToolbar.cpp
+++ b/src/frontends/qt/GuiToolbar.cpp
@@ -339,6 +339,14 @@ public:
DocumentClassConstPtr text_class_;
///
list<FuncRequest> func_requests_;
+ /// Cached menu contents. The menu is rebuilt only when these change,
+ /// since updateTriggered() runs on every toolbar update (i.e. on
+ /// every keypress) and rebuilding leaks the previous actions.
+ vector<docstring> free_fonts_;
+ ///
+ docstring_list selections_;
+ ///
+ bool menu_filled_ = false;
};
@@ -423,14 +431,26 @@ void DynamicMenuButton::updateTriggered()
&& !d->func_requests_.empty()
&& lyx::getStatus(d->func_requests_.front()).enabled());
} else if (menutype == "textstyle-apply") {
- m->clear();
setPopupMode(QToolButton::MenuButtonPopup);
if (!bv) {
+ m->clear();
+ d->free_fonts_.clear();
+ d->menu_filled_ = false;
QToolButton::setIcon(icon_textstyle_apply_);
setEnabled(false);
return;
}
vector<docstring> ffList = bv->cursor().innerText()->getFreeFonts();
+ if (d->menu_filled_ && ffList == d->free_fonts_) {
+ // nothing changed: keep the menu (and its actions) as is
+ QToolButton::setIcon(icon_textstyle_apply_);
+ setEnabled(lyx::getStatus(FuncRequest(LFUN_TEXTSTYLE_APPLY)).enabled()
+ || lyx::getStatus(FuncRequest(LFUN_FONT_DEFAULT)).enabled());
+ return;
+ }
+ d->free_fonts_ = ffList;
+ d->menu_filled_ = true;
+ m->clear();
unsigned int i = 0;
Action * default_act = nullptr;
for (auto const & f : ffList) {
@@ -463,17 +483,29 @@ void DynamicMenuButton::updateTriggered()
setEnabled(lyx::getStatus(FuncRequest(LFUN_TEXTSTYLE_APPLY)).enabled()
|| lyx::getStatus(FuncRequest(LFUN_FONT_DEFAULT)).enabled());
} else if (menutype == "paste") {
- m->clear();
setPopupMode(QToolButton::MenuButtonPopup);
- Action * default_action = new Action(FuncRequest(LFUN_PASTE),
- icon_paste_,
- qt_("Paste"), qt_("Paste"), this);
if (!bv) {
+ m->clear();
+ d->selections_.clear();
+ d->menu_filled_ = false;
setEnabled(false);
- QToolButton::setDefaultAction(default_action);
+ QToolButton::setDefaultAction(new Action(FuncRequest(LFUN_PASTE),
+ icon_paste_, qt_("Paste"),
+ qt_("Paste"), this));
return;
}
docstring_list const sel = cap::availableSelections(&bv->buffer());
+ if (d->menu_filled_ && sel == d->selections_) {
+ // nothing changed: keep the menu (and its actions) as is
+ setEnabled(lyx::getStatus(FuncRequest(LFUN_PASTE)).enabled());
+ return;
+ }
+ d->selections_ = sel;
+ d->menu_filled_ = true;
+ m->clear();
+ Action * default_action = new Action(FuncRequest(LFUN_PASTE),
+ icon_paste_,
+ qt_("Paste"), qt_("Paste"), this);
docstring_list::const_iterator cit = sel.begin();
docstring_list::const_iterator end = sel.end();
==================================================================
Heap fragmentation from per-keystroke paragraph re-breaking
==================================================================
Summary
-------
Once the QAction accumulation described in the first report is fixed, a
second effect remains. It is not a leak: nothing is retained, and every
allocation is properly freed. Each keystroke re-breaks the whole
paragraph, which allocates and frees a burst of Row::Element objects and
their docstrings proportional to the length of that paragraph. The
allocator does not return the freed memory to the system, so RSS climbs
and never comes back down.
The effect is negligible for paragraphs of normal length and becomes
significant only in long paragraphs.
Evidence that nothing is retained
---------------------------------
1. redoParagraph() was instrumented to dump the row cache periodically.
After 800 keystrokes into one paragraph it holds
pms=1 rows=17 elts=17 chars=858
i.e. exactly what an 879-character paragraph needs. The cache does not
grow with the number of keystrokes.
2. With a clean shutdown (save, then lyx-quit, so that every free is
recorded) typing 800 characters leaves 467.00 kB against 466.10 kB for
an idle run - a difference of under 1 kB.
3. Row::Element owns no raw pointers (its docstring and Font members are
RAII, inset and im are non-owning), and redoParagraph() replaces the
row vector wholesale via
pm.rows() = breakParagraph(bigrow);
An earlier measurement suggesting roughly 9900 unfreed allocations for
800 keystrokes was an artifact: it was taken from a killed process, where
everything still in flight is counted as unfreed, and the long-paragraph
run churns far more per keystroke than the short-paragraph one.
Mechanism
---------
Every keystroke goes through singleParUpdate() -> redoParagraph() ->
breakParagraph(), which tokenizes and re-breaks the entire paragraph.
The cost per keystroke is therefore O(paragraph length), and typing a
paragraph of N characters performs O(N^2) allocation work in total. None
of it is retained, but the churn fragments the heap, and glibc keeps the
freed pages.
Call path:
lyx::Row::Element::splitAt(int, int, Row::SplitType, std::vector<>&)
<- TextMetrics::breakParagraph(Row const&)
<- TextMetrics::redoParagraph(long, bool)
<- BufferView::singleParUpdate()
<- BufferView::processUpdateFlags <- updateCurrentView <- dispatch
<- GuiApplication::processKeySym (real keystroke path)
This accounts for 91 percent of the memory attributed to the residual
effect in a heaptrack comparison of a 100-character and an 800-character
typing run.
Measurements
------------
Environment as in the first report (LyX master, Qt 5.15.2, Linux, real X
key events, VmRSS read immediately after the workload). The Action patch
was applied so that only this effect is visible.
same 800 characters typed as 20 paragraphs of 40: 0.3 MB
same 800 characters typed as one paragraph: 5.2 MB
2400 characters typed as one paragraph: 17.3 MB
The decisive comparison is the first pair: identical final text, identical
number of keystrokes, and the difference comes only from the length of the
paragraph being edited - a factor of roughly 17.
Scaling with paragraph length is milder than the O(N^2) work would
suggest (3x the characters gives about 3.3x the RSS), because freed blocks
are largely reused; what remains is fragmentation, not accumulation.
Questions
---------
1. Does singleParUpdate() have to re-break the entire paragraph on every
keystroke, or could row breaking restart from the affected row and
reuse the rows before it? This would remove the O(paragraph length)
per-keystroke cost, and with it both the fragmentation and the
corresponding CPU work.
2. flexible_const_iterator::operator*() in TextMetrics.cpp returns
Row::Element by value, which copies a docstring on every dereference
inside the row-breaking loop. Is that intentional, or could it return a
reference?
Caveats
-------
- The retention behaviour is that of glibc malloc on Linux. How this
translates to the Windows CRT allocator has not been measured, and the
magnitude there may differ substantially in either direction.
- The effect is invisible in "leaked memory" accounting, since nothing is
leaked; it only shows up as process RSS.
Reproduction
------------
Type several hundred characters into a single paragraph without pressing
Enter and watch RSS; then repeat the same number of keystrokes while
breaking the text into short paragraphs. The first case grows markedly
more, although both end with identical document content.
--
lyx-devel mailing list
[email protected]
https://lists.lyx.org/mailman/listinfo/lyx-devel
