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

Optimize a font-heavy document #5071

Merged
merged 2 commits into from
Aug 5, 2014
Merged

Conversation

nnethercote
Copy link
Contributor

I've done some optimizations that reduce the time and space required to view http://www.dynacw.co.jp/Portals/3/fontsamplepdf/sample_4942546800828.pdf.

This patch replaces some vanilla arrays with typed arrays, and avoids
some array copying.

It reduces the peak RSS when viewing
http://www.dynacw.co.jp/Portals/3/fontsamplepdf/sample_4942546800828.pdf
from ~940 MiB to ~750 MiB, and reduces its load time from 83 to 76 ms.
This avoids the creation of over two million array objects when viewing
http://www.dynacw.co.jp/Portals/3/fontsamplepdf/sample_4942546800828.pdf,
and reduces load time from 76 to 73 ms.
@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/7d97c0e44bbb81a/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/8afb31b9dfb845d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/7d97c0e44bbb81a/output.txt

Total script time: 21.12 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/8afb31b9dfb845d/output.txt

Total script time: 23.06 mins

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

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

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

@timvandermeij
Copy link
Contributor

The preview is slower for me (i.e. more than a second rendering time per page extra) than the current master. It it just me or can anyone else also reproduce this?

@nnethercote
Copy link
Contributor Author

I just redid the profiling with Firefox, and I got a 13% improvement in "Overall" time, going from 85ms to 74ms.

@timvandermeij
Copy link
Contributor

Hm, I guess it's just me then, because I just tried again with a cleared cache and a browser restart and it still takes longer with your patch. Strange...

@CodingFabian
Copy link
Contributor

Tim, what document are you testing with? I can try to verify your results.

@timvandermeij
Copy link
Contributor

I'm using the document mentioned in the PR description, i.e. http://www.dynacw.co.jp/Portals/3/fontsamplepdf/sample_4942546800828.pdf. I'm using Nightly 34 on Windows 7. The demo viewer is much faster than the viewer from this PR somehow. I have downloaded the file to make sure network lags are no problem (and opened the file from the local harddisk).

@CodingFabian
Copy link
Contributor

the benchmark is checking what? because when i scroll through the pages they all require noticeable amounts of time. Ill get some numbers...

@CodingFabian
Copy link
Contributor

Ok scrolling down to page 10 gives me these numbers from pdfbug=stats. MacOs Chrome 36:

Page: 10
Page Request 121ms
Rendering    3593ms
Overall      3714ms

With the PR merged I get these numbers:

Page: 10
Page Request 133ms
Rendering    3476ms
Overall      3609ms

looks for me like a good 5% speedup

@timvandermeij
Copy link
Contributor

I did the same thing now. Scrolling down from page 1 to page 10 with pdfBug=Stats enabled. With the current master, I get:

Page Request: 2130ms
Rendering: 3845ms
Overall: 5986ms

With the generated preview above I get:

Page Request: 12343ms
Rendering: 19120ms
Overall: 31465ms

A massive difference. But if both of you cannot replicate those numbers, then it must be something else. Not sure what though: the only difference is the used viewer. The file is loaded from harddisk in both cases and the configuration is exactly the same. I also see nothing in the patch that could cause such a delay.

@CodingFabian
Copy link
Contributor

to be precise: I took master and master+pr.

With preview I get:

Page: 10
Page Request 82ms
Rendering    3346ms
Overall      3428ms

Will ask a windows colleague to check.

@timvandermeij
Copy link
Contributor

I'll also try master and master + PR too. Maybe something is wrong with the preview.

@timvandermeij
Copy link
Contributor

With master (locally) I get:

Page Request 676ms
Rendering 2471ms
Overall 3179ms

With master + PR I get:

Page Request 352ms
Rendering 1478ms
Overall 1830ms

All of a sudden a very large improvement (also visually), whereas the preview was horribly slow. I'm flabbergasted, but since your numbers also confirm this measurement, it must be something with the preview in combination with my configuration.

If I run the test multiple times, the overall time is even lower on average.

@CodingFabian
Copy link
Contributor

There seems to be some windows issues with stability tests. On a Chrome 35 on WIndows 7 I got these numbers:

master

Page: 10
Page Request 100ms
Rendering    4996ms
Overall      5096ms

preview

Page: 10
Page Request 102ms
Rendering    5586ms
Overall      5689ms

And preview was also visually slower

@timvandermeij
Copy link
Contributor

Weird stuff, but at least the PR is not the culprit. Sorry for the noise :)

@nnethercote
Copy link
Contributor Author

Ready to merge?

@nnethercote
Copy link
Contributor Author

@yurydelendik: anything else I need to do here? Thanks.

@yurydelendik yurydelendik self-assigned this Aug 1, 2014
@yurydelendik
Copy link
Contributor

Thank you for the patch.

We still perform bytesToString/stringToArray dance. I think it will be better to do it it three steps: 1) count amount of table entries, padded table sizes, total size of the headers, and allocate single typed array based on that, 2) write all table entries (without creating of temp arrays), 3) write all table data. Also Uint16Array might be changed to Uint8Array.

yurydelendik added a commit that referenced this pull request Aug 5, 2014
@yurydelendik yurydelendik merged commit 46a9a35 into mozilla:master Aug 5, 2014
@nnethercote
Copy link
Contributor Author

I think it will be better to do it it three steps

That sounds promising. I'll continue profiling and see if this part comes up as still significant.

@nnethercote nnethercote deleted the font-savings branch August 6, 2014 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants