gbranden pushed a commit to branch master
in repository groff.
commit b5ede708b92afcb581d0271c92a5863a8264a84a
Author: G. Branden Robinson <[email protected]>
AuthorDate: Tue Mar 17 20:05:42 2026 -0500
[grotty]: Fix Savannah #68145.
[grotty]: Revise dynamic memory allocation strategy. The `tty_printer`
class maintains a member variables `lines` which is a pointer to a
pointer to type `tty_glyph`, implementing a (de facto) rectangular array
of character cells. The strategy that grotty has used to date proved to
perform badly with the new approach to continuous rendering used by
groff's man(7) and mdoc(7) packages in version 1.24.0. If the initial
allocation of 66 lines (rows) of character cells was insufficient
because the input extended the page length, it grew the allocation (and
performed a memcpy(3)) only as much as required to house the vertical
drawing position, plus one line). For a famously lengthy man page like
bash(1), this meant many repeated trips through new/memcpy()/delete
operations, giving the language runtime allocator a lot of work to do.
In an extreme case (formatting 64 copies of bash(1) in sequence), this
led to long runtimes with two-thirds of program time spent in the
kernel.
The new approach is to double the allocation every time it is exceeded,
but also to free that allocation at the end of every page. When
continuously rendering, there will only ever be one page anyway, but
this tactic is more considerate for the (admittedly niche) case of
paginated documents that vary the page length frequently--which is the
approach that groff 1.23.0 and earlier took to continuous rendering,
and that other macro packages or documents might still (but beware of
the sort of problem observed in Savannah #65189).
* src/devices/grotty/tty.cpp: Define new global constant integer,
`default_lines_per_page`, obviating a local literal in the
`tty_printer` constructor.
(class tty_printer): Drop empty body from `begin_page()` declaration.
(tty_printer::tty_printer): Replace open-coded initial allocation
storage backing `lines` member variable with call to `begin_page()`.
(tty_printer::~tty_printer): Stop freeing the storage backing `lines`
here. Instead... (tty_printer::end_page): ...do it here, if its size
grew past beyond default initial allocation.
(tty_printer::add_char): Apply the new reallocation strategy. Use
memset(3) to zero out the new storage before memcpy(3)-ing the
previous contents into (part of) it, eliminating a `for` loop that
manually zeroed out the pointers after the copied storage. While
arguably wasteful to write some memory twice, the runtime can easily
optimize `memset()`, as, for instance, glibc does.[1] Measurement
shows that any reduced efficiency here is lost in the noise of the
vast improvement arising from the allocation size-doubling approach.
(tty_printer::begin_page): Implement. Initially allocate
`default_lines_per_page` rows of `tty_glyph` pointers. Use
`memset(3)` here, too, to zero them out.
[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=46b5e98ef6f1
Fixes <https://savannah.gnu.org/bugs/?68145>. Thanks to dimich for the
report, to Morten Bo Johansen for help reproducing the problem, and to
Deri James for a shrewd pointer in the right direction.
---
ChangeLog | 62 ++++++++++++++++++++++++++++++++++++++++++++++
src/devices/grotty/tty.cpp | 30 ++++++++++++++--------
2 files changed, 82 insertions(+), 10 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 982854267..33a5e07fb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,65 @@
+2026-03-17 G. Branden Robinson <[email protected]>
+
+ [grotty]: Revise dynamic memory allocation strategy. The
+ `tty_printer` class maintains a member variables `lines` which
+ is a pointer to a pointer to type `tty_glyph`, implementing a
+ {de facto} rectangular array of character cells. The strategy
+ that grotty has used to date proved to perform badly with the
+ new approach to continuous rendering used by groff's man(7) and
+ mdoc(7) packages in version 1.24.0. If the initial allocation
+ of 66 lines (rows) of character cells was insufficient because
+ the input extended the page length, it grew the allocation (and
+ performed a memcpy(3)) only as much as required to house the
+ vertical drawing position, plus one line). For a famously
+ lengthy man page like bash(1), this meant many repeated trips
+ through new/memcpy()/delete operations, giving the language
+ runtime allocator a lot of work to do. In an extreme case
+ {formatting 64 copies of bash(1) in sequence}, this led to long
+ runtimes with two-thirds of program time spent in the kernel.
+
+ The new approach is to double the allocation every time it is
+ exceeded, but also to free that allocation at the end of every
+ page. When continuously rendering, there will only ever be one
+ page anyway, but this tactic is more considerate for the
+ {admittedly niche} case of paginated documents that vary the
+ page length frequently--which is the approach that groff 1.23.0
+ and earlier took to continuous rendering, and that other macro
+ packages or documents might still (but beware of the sort of
+ problem observed in Savannah #65189).
+
+ * src/devices/grotty/tty.cpp: Define new global constant
+ integer, `default_lines_per_page`, obviating a local literal in
+ the `tty_printer` constructor.
+ (class tty_printer): Drop empty body from `begin_page()`
+ declaration.
+ (tty_printer::tty_printer): Replace open-coded initial
+ allocation storage backing `lines` member variable with call to
+ `begin_page()`.
+ (tty_printer::~tty_printer): Stop freeing the storage backing
+ `lines` here. Instead...
+ (tty_printer::end_page): ...do it here, if its size grew past
+ beyond default initial allocation.
+ (tty_printer::add_char): Apply the new reallocation strategy.
+ Use memset(3) to zero out the new storage before memcpy(3)-ing
+ the previous contents into (part of) it, eliminating a `for`
+ loop that manually zeroed out the pointers after the copied
+ storage. While arguably wasteful to write some memory twice,
+ the runtime can easily optimize `memset()`, as, for instance,
+ glibc does.[1] Measurement shows that any reduced efficiency
+ here is lost in the noise of the vast improvement arising from
+ the allocation size-doubling approach.
+ (tty_printer::begin_page): Implement. Initially allocate
+ `default_lines_per_page` rows of `tty_glyph` pointers. Use
+ `memset(3)` here, too, to zero them out.
+
+ [1] https://sourceware.org/git/?p=glibc.git;a=commit;\
+ h=46b5e98ef6f1
+
+ Fixes <https://savannah.gnu.org/bugs/?68145>. Thanks to dimich
+ for the report, to Morten Bo Johansen for help reproducing the
+ problem, and to Deri James for a shrewd pointer in the right
+ direction.
+
2026-03-17 G. Branden Robinson <[email protected]>
* src/roff/troff/input.cpp (struct warning_category): Add
diff --git a/src/devices/grotty/tty.cpp b/src/devices/grotty/tty.cpp
index 1e3a66cd6..b50163332 100644
--- a/src/devices/grotty/tty.cpp
+++ b/src/devices/grotty/tty.cpp
@@ -213,7 +213,7 @@ public:
void change_fill_color(const environment * const);
void put_char(output_character);
void put_color(long, int);
- void begin_page(int) { }
+ void begin_page(int);
void end_page(int);
font *make_font(const char *);
};
@@ -288,17 +288,13 @@ tty_printer::tty_printer() : cached_v(0)
&dummy, 5);
(void) has_color(0, color::MAX_COLOR_VAL, color::MAX_COLOR_VAL,
&dummy, 6);
- nlines = 66;
- lines = new tty_glyph *[nlines];
- for (int i = 0; i < nlines; i++)
- lines[i] = 0;
+ begin_page(0 /* dummy */);
is_continuously_underlining = false;
}
tty_printer::~tty_printer()
{
current_lineno = 0; // At this point, we've read all the input.
- delete[] lines;
}
void tty_printer::make_underline(int w)
@@ -392,12 +388,14 @@ void tty_printer::add_char(output_character c, int w,
vpos = v / font::vert;
if (vpos > nlines) {
tty_glyph **old_lines = lines;
- lines = new tty_glyph *[vpos + 1];
+ // If we exceed the previous page length, double the size so that
+ // we don't thrash the allocator. See Savannah #68145.
+ int new_nlines = nlines * 2;
+ lines = new tty_glyph *[new_nlines];
+ memset(lines, 0, new_nlines * sizeof(tty_glyph *));
memcpy(lines, old_lines, nlines * sizeof(tty_glyph *));
- for (int i = nlines; i <= vpos; i++)
- lines[i] = 0;
delete[] old_lines;
- nlines = vpos + 1;
+ nlines = new_nlines;
}
// Note that the first output line corresponds to groff
// position font::vert.
@@ -760,6 +758,16 @@ void tty_printer::put_color(long color_index, int back)
}
}
+// We could make this 70 where ISO paper formats are used.
+const int default_lines_per_page = 66;
+
+void tty_printer::begin_page(int)
+{
+ nlines = default_lines_per_page;
+ lines = new tty_glyph *[nlines];
+ memset(lines, 0, nlines * sizeof(tty_glyph *));
+}
+
// The possible Unicode combinations for crossing characters.
//
// ' ' = 0, ' -' = 4, '- ' = 8, '--' = 12,
@@ -946,6 +954,8 @@ void tty_printer::end_page(int page_length)
for (; last_line < lines_per_page; last_line++)
putchar('\n');
}
+ if (nlines > default_lines_per_page)
+ delete[] lines;
}
font *tty_printer::make_font(const char *nm)
_______________________________________________
groff-commit mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/groff-commit
