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

Reduce ASCII checks in makeInlineImage(). #5187

Merged
merged 1 commit into from
Aug 15, 2014

Conversation

nnethercote
Copy link
Contributor

makeInlineImage() has a "are the next five chars ASCII?" check which is
run after an "EI" sequence has been found. This check involves the
creation of a new object because peekBytes() calls subarray().

Unfortunately, the check is currently run on whitespace chars even when
an "EI" sequence has not yet been found, i.e. when it's not needed. For
the PDF in #2618, there are over 820,000 such checks.

This change reworks the relevant loop so that the check is only done
once an "EI" sequence has been seen. This reduces the number of checks
to 157,000, and speeds up rendering by somewhere between 2% and 7% (the
measurements are noisy).

state = 0;
break; // some binary stuff found, resetting the state
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about this break (and one below) removal, does it improve the performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

with the break removed it would always scan the 5 chars, wouldnt it?

Copy link
Contributor

Choose a reason for hiding this comment

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

.. but it might open road for some jit optimizations, that's why I'm wondering. Will it be beneficial to add a constant 5 instead of "followingBytes.length" to the for-loop then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the break back in. I don't know what the JITs will do, but I can put the 5 in its own variable.

makeInlineImage() has a "are the next five chars ASCII?" check which is
run after an "EI" sequence has been found. This check involves the
creation of a new object because peekBytes() calls subarray().

Unfortunately, the check is currently run on whitespace chars even when
an "EI" sequence has not yet been found, i.e. when it's not needed. For
the PDF in mozilla#2618, there are over 820,000 such checks.

This change reworks the relevant loop so that the check is only done
once an "EI" sequence has been seen. This reduces the number of checks
to 157,000, and speeds up rendering by somewhere between 2% and 7% (the
measurements are noisy).
@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/cdb4df9f9e7f675/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/b53e64276c6bc14/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/cdb4df9f9e7f675/output.txt

Total script time: 19.76 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/b53e64276c6bc14/output.txt

Total script time: 22.52 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@CodingFabian
Copy link
Contributor

looks good to me.

yurydelendik added a commit that referenced this pull request Aug 15, 2014
Reduce ASCII checks in makeInlineImage().
@yurydelendik yurydelendik merged commit 992e761 into mozilla:master Aug 15, 2014
@yurydelendik
Copy link
Contributor

Thank you for the patch

@nnethercote nnethercote deleted the EI-checking branch August 15, 2014 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants