Add global caching, for /Resources without blend modes, and use it to reduce repeated fetching/parsing in PartialEvaluator.hasBlendModes
#12583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snuffleupagus
force-pushed
the
nonBlendModesSet
branch
from
68fd87a to
ecabd75
Compare
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/a1c9020f8670b34/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/547415f424abb25/output.txt |
Snuffleupagus
force-pushed
the
nonBlendModesSet
branch
from
ecabd75 to
efee0d8
Compare
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/547415f424abb25/output.txt Total script time: 25.32 mins
Image differences available at: http://54.67.70.0:8877/547415f424abb25/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/a1c9020f8670b34/output.txt Total script time: 28.52 mins
Image differences available at: http://3.101.106.178:8877/a1c9020f8670b34/reftest-analyzer.html#web=eq.log |
… reduce repeated fetching/parsing in `PartialEvaluator.hasBlendModes` The `PartialEvaluator.hasBlendModes` method is necessary to determine if there's any blend modes on a page, which unfortunately requires *synchronous* parsing of the /Resources of each page before its rendering can start (see the "StartRenderPage"-message). In practice it's not uncommon for certain /Resources-entries to be found on more than one page (referenced via the XRef-table), which thus leads to unnecessary re-fetching/re-parsing of data in `PartialEvaluator.hasBlendModes`. To improve performance, especially in pathological cases, we can cache /Resources-entries when it's absolutely clear that they do not contain *any* blend modes at all[1]. This way, subsequent `PartialEvaluator.hasBlendModes` calls can be made significantly more efficient. This patch was tested using the PDF file from issue 6961, i.e. https://github.com/mozilla/pdf.js/files/121712/test.pdf: ``` [ { "id": "issue6961", "file": "../web/pdfs/issue6961.pdf", "md5": "a80e4357a8fda758d96c2c76f2980b03", "rounds": 100, "type": "eq" } ] ``` which gave the following results when comparing this patch against the `master` branch: ``` -- Grouped By browser, page, stat -- browser | page | stat | Count | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05) ------- | ---- | ------------ | ----- | ------------ | ----------- | ---- | ------ | ------------- firefox | 0 | Overall | 100 | 1034 | 555 | -480 | -46.39 | faster firefox | 0 | Page Request | 100 | 489 | 7 | -482 | -98.67 | faster firefox | 0 | Rendering | 100 | 545 | 548 | 2 | 0.45 | firefox | 1 | Overall | 100 | 912 | 428 | -484 | -53.06 | faster firefox | 1 | Page Request | 100 | 487 | 1 | -486 | -99.77 | faster firefox | 1 | Rendering | 100 | 425 | 427 | 2 | 0.51 | ``` --- [1] In the case where blend modes *are* found, it becomes a lot more difficult to know if it's generally safe to skip /Resources-entries. Hence we don't cache anything in that case, however note that most document/pages do not utilize blend modes anyway.
Snuffleupagus
force-pushed
the
nonBlendModesSet
branch
from
efee0d8 to
082cd8f
Compare
timvandermeij
approved these changes
Nov 5, 2020
|
Looks good; thank you for doing this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
PartialEvaluator.hasBlendModesmethod is necessary to determine if there's any blend modes on a page, which unfortunately requires synchronous parsing of the /Resources of each page before its rendering can start (see the "StartRenderPage"-message).In practice it's not uncommon for certain /Resources-entries to be found on more than one page (referenced via the XRef-table), which thus leads to unnecessary re-fetching/re-parsing of data in
PartialEvaluator.hasBlendModes.To improve performance, especially in pathological cases, we can cache /Resources-entries when it's absolutely clear that they do not contain any blend modes at all[1]. This way, subsequent
PartialEvaluator.hasBlendModescalls can be made significantly more efficient.This patch was tested using the PDF file from issue #6961, i.e. https://github.com/mozilla/pdf.js/files/121712/test.pdf:
which gave the following results when comparing this patch against the
masterbranch:[1] In the case where blend modes are found, it becomes a lot more difficult to know if it's generally safe to skip /Resources-entries. Hence we don't cache anything in that case, however note that most document/pages do not utilize blend modes anyway.