Slightly delay rendering during scrolling in the viewer (bug 1854145)

[Snuffleupagus]

This patch tries to improve a use-case that's always performed somewhat poorly in the default viewer, i.e. scrolling quickly through long and complex PDF documents.
For an initial implementation I've purposely tried to make these new delays as small as possible, while still improving cases such as e.g. bug 1854145, to hopefully avoid regressing perceived performance for all PDF documents in the default viewer.

Please refer to the inline code-comments for additional details.

[calixteman]

Interesting, I worked on the same issue but with a totally different approach.
For example, in using the pdf in https://bugzilla.mozilla.org/show_bug.cgi?id=1854145 and in scrolling it with the keyboard to page 300, with a mac (M1), page 300 is rendered in around 3 seconds.

[Snuffleupagus]

Interesting, I worked on the same issue but with a totally different approach.

Oh, how did you implement that (at a high level)?

(I've been thinking about this issue for a while, and have had problems coming up with a generally good solution that improves handling of huge documents without regressing performance for "normal" document and also doesn't break some edge-case.)

For example, in using the pdf in https://bugzilla.mozilla.org/show_bug.cgi?id=1854145 and in scrolling it with the keyboard to page 300, with a mac (M1), page 300 is rendered in around 3 seconds.

I mostly tested mouse-scrolling locally, and it unfortunately seems that this patch doesn't handle keyboard-scrolling as well (since there's a larger "delay" between the update-calls in that case).
Hence this patch probably isn't such a great idea unfortunately, since we can't just increase these timeouts without affecting things negatively overall :-(

[calixteman]

Tell me what do you think about:
#17403

[Snuffleupagus]

Tell me what do you think about:
#17403

While something along those lines is likely (a lot) better, it does seem significantly more complex and risky though (i.e. it'd take some time to review.)
Given that "everything" in the MessageHandler is being wrapped in a setTimeout(..., 0)-call, what overall performance impact will this have (in a "normal" PDF document) since this will add a small delay to the start of all worker-thread parsing?
Also, what happens if the following order of events occur in quick succession for a page: Trigger rendering, cancel rendering, re-trigger rendering; does that still work "reasonably"? (For the second rendering op, the pageId may still be "cancelled".)

[calixteman]

Tell me what do you think about:
#17403

While something along those lines is likely (a lot) better, it does seem significantly more complex and risky though (i.e. it'd take some time to review.) Given that "everything" in the MessageHandler is being wrapped in a setTimeout(..., 0)-call, what overall performance impact will this have (in a "normal" PDF document) since this will add a small delay to the start of all worker-thread parsing?

Yes, we could start to setTimeout... the execution of the tasks after a specific message which could be sent just before the rendering of the first page.
That said, with the mentioned pdf, if you slightly scroll the first page, after few millis, the page is scrolled back to its initial position (it's a way more visible in changing the value in the setTimeout). Interestingly, it's reproducible with master (you just need slightly scroll very quickly). So my patch highlights an existing "bug", but it shows that the perfs at startup are not the same.

Also, what happens if the following order of events occur in quick succession for a page: Trigger rendering, cancel rendering, re-trigger rendering; does that still work "reasonably"? (For the second rendering op, the pageId may still be "cancelled".)

The pageIdshoud be incremented so we shouldn't have any conflicts.

I wrote this patch almost to have a POC (based on an idea given by the bug reporter in 1866296).
That said, you know that stuff a way better than me, so don't worry to "steal" my patch to improve it if you've any ideas to make it better.

[Snuffleupagus]

Yes, we could start to setTimeout... the execution of the tasks after a specific message which could be sent just before the rendering of the first page.

That's all starting to sound even more complicated unfortunately...

That said, with the mentioned pdf, if you slightly scroll the first page, after few millis, the page is scrolled back to its initial position (it's a way more visible in changing the value in the setTimeout). Interestingly, it's reproducible with master (you just need slightly scroll very quickly). So my patch highlights an existing "bug", but it shows that the perfs at startup are not the same.

Huh, that sounds strange (and I'm not sure that I fully understand what you're describing).

The pageIdshoud be incremented so we shouldn't have any conflicts.

The new pageIds are constant for each PDFPageProxy-instance (i.e. each page) and those are cached in the API. Hence the pageId won't change for two different render-calls for the same page and consequently I believe that my previous comment still applies here.

Thinking about all of this a bit more, I'm not sure that simply "ignoring" some sendWithPromise-calls as well (in the MessageHandler) is correct/safe either. Can't that easily lead to some Promises, as returned from the API, remaining pending indefinitely?
Most likely you want to limit this new cancelling to only the ReadableStreams, used with sendWithStream, since that's really the only thing that can/needs to be aborted as far as I can tell.

That said, you know that stuff a way better than me, so don't worry to "steal" my patch to improve it if you've any ideas to make it better.

I don't know about "better", I've just been around the project a bit longer and thus remembers how difficult accounting for various edge-cases can be in parts of the code-base.
(Given the complexity of all this, I don't know when I'd find the time to look into this unfortunately. Considering the development pace these days, trying to keep up with review requests usually takes up most of my spare time :-)

[Snuffleupagus]

For this PR I've tried playing with the constants a little bit and it now seems to work OK with both mouse- and keyboard-scrolling at least in my tests. (And it should be pretty much risk free, since we'll just defer render-calls a tiny bit in the viewer.)

/botio-linux preview

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Received

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

Live output at: http://54.241.84.105:8877/1bbdb409afedeeb/output.txt

[moz-tools-bot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.241.84.105:8877/1bbdb409afedeeb/output.txt

Total script time: 1.44 mins

Published

• Viewer: http://54.241.84.105:8877/1bbdb409afedeeb/web/viewer.html
• Viewer (legacy): http://54.241.84.105:8877/1bbdb409afedeeb/legacy/web/viewer.html