Repository navigation
Walk only items that cover the current row - #22
TomHoenderdos wants to merge 1 commit into
Conversation
bettio
left a comment
There was a problem hiding this comment.
PR after PR we are getting close :)
I have a few comments on possible improvements on this PR:
-
Rebase on
main. #23 replaceddo_update()insdl_display/display.cwith a plain full redraw, so that hunk conflicts (GitHub shows it as dirty). Everything else applies as is. The resolution I built and ran is:BaseDisplayItem **row = malloc(sizeof(BaseDisplayItem *) * len); if (UNLIKELY(len > 0 && !row)) { fprintf(stderr, "do_update: failed to alloc row\n"); display_items_delete(items, len); return; } for (int ypos = 0; ypos < screen->h; ypos++) { size_t row_len = display_items_row(items, len, ypos, row); int xpos = 0; while (xpos < screen->w) { xpos += draw_x(xpos, ypos, row, row_len); } } free(row); display_items_delete(items, len);
Note the
display_items_delete()on the failure path: onmainthe items are no longer kept inprev_items, so returning without it would leak them. (It becomes moot with point 2.) -
Consider dropping the row array altogether. The same
malloc/ check /freeblock now appears in five drivers plus the test. Instead of a separate array, the per-row list can be threaded through the items themselves: add a scratch link to the item,struct BaseDisplayItem { ... // Scratch link used while rendering: the next item covering the current row. struct BaseDisplayItem *next; };
let
display_items_row(items, len, ypos)return the head of that list (append with aBaseDisplayItem **link, terminate with NULL), and let every*_draw_x()takeBaseDisplayItem *headand walkitem->next. I tried it on top of your branch: identical output on the same 2000 random lists, same speed within noise (the walk already loads one pointer per item either way), and the same 4 bytes per item the array costs (56 -> 60 bytes on ESP32). What it removes is an allocation and an out-of-memory path per update in every driver, thelen == 0special case, and theitems_lenparameter ofdraw_x(). The field must be documented as rendering scratch, like the row memo of your shape items, and must never take part in item comparison. If you would rather not touch the struct, the other way to get a single failure path is to allocate the pointer array in the samemallocas the items insidedisplay_items_new_list(). -
Subject line. Per the AtomVM style guide the subject starts with one of the standard verbs; "Walk only items that cover the current row" reads well but "Optimize draw_x with per-row item lists" (or similar) would match the convention.
Every draw_x call tested every item's bounding box against the row, once per run of pixels. On the badge a racing game with about 89 items, of which about 10 cover any row, took 21 ms per frame to draw. The drivers now link the items that cover each row through a new next field with display_items_row(), and draw_x walks only that list. The field is rendering scratch and never compared. Nothing is allocated per update. Output is unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Tom Hoenderdos <tomhoenderdos@gmail.com>
fe50db3 to
9a22547
Compare
|
@bettio thanks! Claude and I went through your points, with the same numbers as yours. [1] Rebase: done ✅The branch now starts from the current [2] Per-row list through the items: done ✅Each item has a [3] Subject: done ✅"Optimize draw_x with per-row item lists". Numbers from the deviceGoatmire badge. "Before" is on Busy scenes gain the most, and nothing gets slower: building the list on small scenes costs nothing we can measure. |
Summary
Every
draw_xcall walked the whole display list and tested each item's bounding box against the row, once per run of pixels. The drivers now build, once per row, the list of items that cover it withdisplay_items_row()and pass that to the renderers'draw_x, so items above or below the row cost one bounding box test per line instead of one per run. Output is unchanged.All drivers (DCS LCD, e-paper, memory LCD, OLED, SDL) and renderers (dcs_lcd, mono, epaper, SDL) are updated. The row pointer array is allocated once per update with the items.
Tested on hardware
ESP32-S3 badge, ST7789 320×240 RGB565 at 80 MHz SPI, ESP-IDF 5.5.2, a racing game with 71–104 items per frame (it uses the shape primitives from #18), with the profiling from #18:
Other testing
tests/items(ASan+UBSan) on the host, which renders throughdcs_lcd_draw_x()with the row lists.main,ufont_manager_registerarity).This touches the same lines of
dcs_lcd_display_driver.canddisplay_items.has #18; whichever is merged second will be rebased.🤖 Generated with Claude Code