Convert Catalog.getPageDict to an async method
#11312
Conversation
This makes it possible to remove the internal `next` helper function, and also gets rid of the need to manually resolve/reject a `PromiseCapability`.
As we've seen in numerous other cases, avoiding unnecessary function calls is never a bad thing (even if the effect is probably tiny here). With these changes we also avoid potentially two back-to-back `isDict` checks when evaluating possible Page nodes, and can also no longer accidentally pick a dictionary with an incorrect /Type.
Snuffleupagus
force-pushed
the
async-getPageDict
branch
from
e5f103a to
79d7c00
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/84f317acfc45ce7/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/b6d98b39c223b9f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/84f317acfc45ce7/output.txt Total script time: 18.69 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/b6d98b39c223b9f/output.txt Total script time: 26.77 mins
|
|
Thank you for refactoring this! I found the |
Yeah, the old setup was essentially a recursive function which was called asynchronously. This was also a case where the use of a I suppose the one criticism you could have of this patch, is that it's adding asynchronous behaviour inside of a loop. However I think all the skipped function calls and the general clean-up outweighs that. |
|
Sigh, it seems that I forgot to test this with really large files :-( With a large file taken from an old GitHub issue (warning, the file is 16 MB): https://github.com/mozilla/pdf.js/files/876321/kjv.pdf there's now a considerable "Page Request" regression for the second page upon document load; from 2000 -> 3000 ms which obviously doesn't look good. In light of this I don't believe that it's a good idea keep these changes unfortunately, since the new way in which this code is asynchronous obviously performs worse in some cases; sorry about the mess here! |
This makes it possible to remove the internal
nexthelper function, and also gets rid of the need to manually resolve/reject aPromiseCapability.