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

Investigate if we could use some caches based on WeakRef #18148

Open
calixteman opened this issue May 22, 2024 · 6 comments
Open

Investigate if we could use some caches based on WeakRef #18148

calixteman opened this issue May 22, 2024 · 6 comments

Comments

@calixteman
Copy link
Contributor

For the context, in Fenix, several users reported some freeze when zooming/scrolling a pdf:

So my idea is too try to release memory when needed because devices don't always have a lot of memory and because afaik Android OS is trying to smartly manage it (for example in killing some apps in background).
During the rendering of a page, we could keep a normal reference on each object to avoid to accidentally remove them. When the rendering is done we could just move them (maybe after a delay we could configure through an option which could be zero with Fenix and 30s for Firefox) and keep a WeakRef (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_management#weakrefs_and_finalizationregistry) on the moved objects.

@Snuffleupagus, @timvandermeij, any positive/negative thoughts ?

@Snuffleupagus
Copy link
Collaborator

Note that we already have code that will conditionally clear various main-thread page-data caches once rendering completes, see

pdf.js/src/display/api.js

Lines 1470 to 1478 in b7b8e5e

const complete = error => {
intentState.renderTasks.delete(internalRenderTask);
// Attempt to reduce memory usage during *printing*, by always running
// cleanup immediately once rendering has finished.
if (this._maybeCleanupAfterRender || intentPrint) {
this.#pendingCleanup = true;
}
this.#tryCleanup(/* delayed = */ !intentPrint);
and

pdf.js/src/display/api.js

Lines 2826 to 2833 in b7b8e5e

case "Image":
pageProxy.objs.resolve(id, imageData);
// Heuristic that will allow us not to store large data.
if (imageData?.dataLen > MAX_IMAGE_SIZE_TO_CACHE) {
pageProxy._maybeCleanupAfterRender = true;
}
break;

So one thing that you could try experimenting with is setting this.#pendingCleanup = true in GECKOVIEW-builds, once rendering completes, since that might help without having to adding a bunch more complexity (e.g. ´WeakRef`s to the code)?

This should help reclaim memory faster, using existing code; obviously at the expense of needing to re-parse pages more often.

@calixteman
Copy link
Contributor Author

The main advantage of the WeakRef approach is that we'd be able to adapt to the device, I mean if the device as a low memory (even if it has a lot of memory but a lot of open apps) then a GC is triggered when potentially GC'd be less frequent if the device has enough memory. For example, myself I'm not able reproduce the bugs we've because I've a recent device with a lot of memory and in my case I'd prefer rendering performance over memory use.
In general, the problem with hard-coded values is they're well-tuned for some device/usage/users... when they aren't for some others.
That said in term of complexity, we should likely just have a cache object with all the caching logic inside instead of spreading some little piece everywhere, which could help us to make some experimentations.

@marco-c
Copy link
Contributor

marco-c commented May 22, 2024

Trying the WeakRef approach seems most promising for a good balance between perf / memory.

That said, I can reproduce the bug, if you have any quick patch I could try it out to confirm a potential cause for the problem.

@Snuffleupagus
Copy link
Collaborator

Since #18148 (comment) sounds a little bit like it's written by an AI, please note that you're not allowed to write code using AI for Mozilla projects (based on a general question about AI in the Mozilla Matrix "developers" channel).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@marco-c @timvandermeij @Snuffleupagus @calixteman and others