Skip to content
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.

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

Don't store page-level data, in the API, after cleanup has run (bug 1854145) #17106

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

For large/complex images it's possible that the image-data arrives in the API after the page has been scrolled out-of-view and thus been cleaned-up. In this case we obviously shouldn't cache such page-level data, since it'll first of all be unused and secondly can increase memory usage a lot.
Also, ensure that we immediately release any ImageBitmap data in this case to help reclaim memory faster.

@Snuffleupagus
Copy link
Collaborator Author

This is based on the nice idea in https://bugzilla.mozilla.org/show_bug.cgi?id=1854145#c12, however please note that I've not (yet) run any tests locally and I've also not really spent a lot of time thinking through these changes.

@calixteman
Copy link
Contributor

I just tested locally on a mac and it works pretty well.
I scrolled very quickly 110 pages and then I ran the profiler and I can observe a huge difference in term of memory use:
https://share.firefox.dev/3rCq2jX

@Snuffleupagus
Copy link
Collaborator Author

/botio test

@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 11, 2023 07:07
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@Snuffleupagus Snuffleupagus marked this pull request as draft October 11, 2023 09:35
…854145)

For large/complex images it's possible that the image-data arrives in the API *after* the page has been scrolled out-of-view and thus been cleaned-up. In this case we obviously shouldn't cache such page-level data, since it'll first of all be unused and secondly can increase memory usage *a lot*.
Also, ensure that we *immediately* release any `ImageBitmap` data in this case to help reclaim memory faster.
@Snuffleupagus Snuffleupagus changed the title Don't store page-level data, in the API, unless rendering is ongoing (bug 1854145) Don't store page-level data, in the API, after cleanup has run (bug 1854145) Oct 11, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 11, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 11, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 11, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Oct 11, 2023
@Snuffleupagus
Copy link
Collaborator Author

Given that we abort worker-thread parsing with a delay, to avoid a bunch of re-parsing on zooming and rotation in the viewer, it occurred to me that we shouldn't just check if rendering is "ongoing" but actually ensure that cleanup has successfully run when we decide to ignore any image-data here.

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/73b4f7f6d027bf2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/70ae3b870b97db9/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/73b4f7f6d027bf2/output.txt

Total script time: 24.52 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 15
  different first/second rendering: 3

Image differences available at: http://54.241.84.105:8877/73b4f7f6d027bf2/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/70ae3b870b97db9/output.txt

Total script time: 36.93 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 23

Image differences available at: http://54.193.163.58:8877/70ae3b870b97db9/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 11, 2023 10:44
@calixteman
Copy link
Contributor

On Windows 11, I scrolled around page 120 in keeping pressed PageDown.
Without your patch:
https://share.firefox.dev/3M2pgU7
and with:
https://share.firefox.dev/3tCR3UE
so it works as expected.

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@Snuffleupagus Snuffleupagus merged commit 158ab5b into mozilla:master Oct 12, 2023
3 checks passed
@Snuffleupagus Snuffleupagus deleted the bug-1854145 branch October 12, 2023 11:26
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 7, 2023
github-actions bot pushed a commit to Floorp-Projects/Floorp that referenced this pull request Dec 18, 2023
Ponchale added a commit to goastian/midori-desktop that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants