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.

Sign up for GitHub

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

Jump to bottom

Refactors showText: split type3, remove showSpacedText #4832

Merged
merged 1 commit into from
May 29, 2014
Merged

Refactors showText: split type3, remove showSpacedText #4832

merged 1 commit into from
May 29, 2014

Conversation

yurydelendik
Copy link
Contributor

about 4% increase in speed of rendering:

browser        | stat         | Baseline(ms) | Current(ms) | +/- | %    | Result(P<.05)
---------------------------------------------------------------------------------------
FirefoxNightly | Overall      | 118          | 113         | 5   | 3.96 |              
FirefoxNightly | Page Request | 4            | 4           | 0   | 1.97 |              
FirefoxNightly | Rendering    | 114          | 110         | 5   | 4.03 |

https://gist.github.com/yurydelendik/998bda55ad2868ce73bb

@yurydelendik yurydelendik changed the title Refactors showText: split type3, remove showSpacedText [WIP] Refactors showText: split type3, remove showSpacedText May 23, 2014
@yurydelendik
Copy link
Contributor Author

On windows:

-- Grouped By browser, stat --
browser | stat | Baseline(ms) | Current(ms) | +/- | % | Result(P<.05)
---------------------------------------------------------------------------------
chrome | Overall | 89 | 75 | 14 | 15.88 | faster
chrome | Page Request | 5 | 5 | 0 | 0.13 |
chrome | Rendering | 84 | 70 | 14 | 16.79 | faster
firefox | Overall | 238 | 223 | 14 | 6.06 |
firefox | Page Request | 5 | 5 | 0 | 1.69 |
firefox | Rendering | 233 | 218 | 14 | 6.15 |

@Snuffleupagus
Copy link
Collaborator

The general structure looks good, but I'll need to go through this at least once more before I'm comfortable with merging it. (There's a few things I don't completely understand yet.)

/botio test

@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/c288e7d2157b071/output.txt

@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/7d33185a646e24b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

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

Total script time: 2.71 mins

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

Image differences available at: http://107.22.172.223:8877/7d33185a646e24b/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux)

Failed

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

Total script time: 26.42 mins

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

Image differences available at: http://107.21.233.14:8877/c288e7d2157b071/reftest-analyzer.html#web=eq.log

Snuffleupagus
Snuffleupagus reviewed May 25, 2014
View reviewed changes
src/display/canvas.js
x += fontDirection * wordSpacing;
continue;
} else if (isNum(glyph)) {
x += -glyph * fontSize * 0.001;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is perhaps textHScale missing here? I.e. shouldn't the line read:

x += -glyph * fontSize * 0.001 * textHScale;

Edit: This would be consistent with the Type3 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do that below ~L1433

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see!
I was just trying to find a possible reason for all the movement in the tests, or is that expected given the code changes?

@yurydelendik yurydelendik changed the title [WIP] Refactors showText: split type3, remove showSpacedText Refactors showText: split type3, remove showSpacedText May 28, 2014
@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/5c105a0b579537c/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/172248752f6012d/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/5c105a0b579537c/output.txt

Total script time: 23.73 mins

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

Image differences available at: http://107.22.172.223:8877/5c105a0b579537c/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/172248752f6012d/output.txt

Total script time: 26.20 mins

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

Image differences available at: http://107.21.233.14:8877/172248752f6012d/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

Given that the movements in the tests are really small, they can probably be explained by the fact that the handling of some text OPS were moved from canvas.js to evaluator.js. Another contributing factor is likely to be the re-factored (removed) showSpacedText in canvas.js.

As can also be seen in the test results, all the differences are not in text but some of them also affect other drawing operations. This would then be similar to #4683 (comment), meaning that the differences should be fine.

Based on IRC discussions, I think that this PR should be good to go!

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 1

Live output at: http://107.22.172.223:8877/73e4b622d97f295/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/73e4b622d97f295/output.txt

Total script time: 25.16 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)

Success

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

Total script time: 26.13 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

Snuffleupagus added a commit that referenced this pull request May 29, 2014
@Snuffleupagus
Merge pull request #4832 from yurydelendik/showtext
7e6cdc7 
Refactors showText: split type3, remove showSpacedText
@Snuffleupagus Snuffleupagus merged commit 7e6cdc7 into mozilla:master May 29, 2014
@fkaelberer fkaelberer mentioned this pull request May 31, 2014
Merged
@Snuffleupagus Snuffleupagus mentioned this pull request Aug 28, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yurydelendik @Snuffleupagus @pdfjsbot