Skip to content

Sharpdisplay: reuse supervisor heap #3498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 22, 2020
Merged

Sharpdisplay: reuse supervisor heap #3498

merged 1 commit into from
Oct 22, 2020

Conversation

jepler
Copy link

@jepler jepler commented Oct 1, 2020

Due to an existing limitation of being unable to re-allocate a freed supervisor heap entry, framebuffer displays used more memory than necessary, particularly on the second and subsequent runs: The preserved-over-soft-reset display has its framebuffer moved to the supervisor heap, but we release that display with release_displays() and then create a fresh one which places its framebuffer on the GC heap.

A newly exposed problem, where framebuffer displays would cause a leak of the terminal grid storage, was also found and fixed.

With this change, sharp displays will re-use the same supervisor allocation as long as the WxH of the display matches.

A similar change should be made for RGBMatrix, however at this moment I did not want to take the time to implement and test it. It consists of switching the order of preference for allocation to try supervisor first and gc second, then fixing whatever breaks.

This requires #3482 + #3497 in order to be fully effective, so it should be merged only after those go in.

@jepler jepler mentioned this pull request Oct 1, 2020
Closed
tannewt
tannewt reviewed Oct 1, 2020
View reviewed changes
@jepler jepler mentioned this pull request Oct 7, 2020
Merged
@cwalther
Copy link

cwalther commented Oct 9, 2020

I investigated why #3482 causes a problem with framebuffer displays. It’s not a bug in #3482 (phew), but as you say it’s actually the double call of supervisor_start_terminal that only starts causing problems once the ability to reuse supervisor allocations is there. The reason is simple: In your test case (I used the setup_display.py from #3482 (review)), the double call is initiated from Python, therefore without the ability to reuse supervisor allocations, both calls fail to allocate from the supervisor and allocate from the GC heap instead. The leak caused by the second call therefore happens on the GC heap, and is not a leak because the GC can deal with that. With #3482, the first call can reuse the supervisor allocation created by supervisor_display_move_memory after a previous run and then freed by displayio.release_displays(). The second call (which still fails to allocate from the supervisor because there’s neither a second hole to be reused nor free space) then leaks that allocation.

The first commit of this PR has no bearing on that, the framebuffer is always handled correctly (before #3482, between #3482 and this PR, with this PR).

The second commit of this PR seems to be a correct fix for the terminal issue, but as you say, it would probably be even better to avoid the double call (and either keep this or make sure there can be no other way of getting two calls in a row).

@cwalther cwalther mentioned this pull request Oct 21, 2020
Closed
@jepler
sharpdisplay: Re-use supervisor allocations if possible
b520498 
This is enabled by adafruit#3482

I was unable to determine why previously I had added sizeof(void*)
to the GC heap allocation, so I removed that code as a mistake.
@jepler jepler force-pushed the sharpdisplay-reuse-supervisor-heap branch from 63c3c01 to b520498 Compare October 22, 2020 17:37
@jepler jepler requested a review from tannewt October 22, 2020 17:37
@jepler
Copy link
Author

jepler commented Oct 22, 2020

I think this can go in now. I didn't do any re-testing today, though.

cwalther
cwalther approved these changes Oct 22, 2020
View reviewed changes
Copy link

@cwalther cwalther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me and I’ve given it a quick test now that I have a Sharp display (thanks to @deshipu), it works fine and is effective in saving memory on the second and subsequent runs.

tannewt
tannewt approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tannewt tannewt merged commit 64c6d6d into adafruit:main Oct 22, 2020
@jepler jepler deleted the sharpdisplay-reuse-supervisor-heap branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

@cwalther cwalther cwalther approved these changes

Assignees
No one assigned
Labels
displayio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jepler @cwalther @tannewt @hierophect