minor improvements and code cleanup for canvas.js #5446

CodingFabian · 2014-10-26T17:25:28Z

These changes are by products of my investigations on slow pdfs.
The impact disappears in the noise, but I still would like to merge them:

Direct property access using "[foo]" instead of "foo in"
making FULL_CHUNK_HEIGHT a global variable
not calculating patternFill every time when read, but set on write
replaced fullChunk/partialChunk calculation.

What I find interesting is that a (possibly flawed) benchmark for the
floor+ceil code vs the modulo variant shows so dramatic differences:
http://jsperf.com/floor-and-ceil-or-modulo

I had expected that modulo is faster than invoking the methods, but
the safari 8 result blows my mind. Additionally, I am wondering about
the reason for the Firefox regression for a calculation which has a
modulo !== 0

Snuffleupagus · 2014-10-26T17:38:52Z

src/display/canvas.js


      var glyph = this.processingType3;

-      if (COMPILE_TYPE3_GLYPHS && glyph && !('compiled' in glyph)) {
-        var MAX_SIZE_TO_COMPILE = 1000;
+      if (COMPILE_TYPE3_GLYPHS && glyph && glyph['compiled'] !== undefined) {


The last comparison looks off. The old code is !('compiled' in glyph), so shouldn't it be !glyph['compiled'] in the new code?

correct. I reverted this once and seemed to have missed that when retyping. thanks!

timvandermeij · 2014-12-16T18:54:58Z

/botio test

pdfjsbot · 2014-12-16T18:54:58Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/6ec16c70295539f/output.txt

pdfjsbot · 2014-12-16T18:54:58Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/6bd5b60d74ddd8d/output.txt

pdfjsbot · 2014-12-16T19:12:04Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/6bd5b60d74ddd8d/output.txt

Total script time: 17.10 mins

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

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

pdfjsbot · 2014-12-16T19:17:20Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/6ec16c70295539f/output.txt

Total script time: 22.36 mins

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

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

CodingFabian · 2014-12-16T19:21:23Z

hu, the image diffs are interesting. need to check whats with that.

CodingFabian · 2014-12-16T19:48:22Z

okay, pretty stupid math error.
@timvandermeij can you retrigger the /botio test ?

Snuffleupagus · 2014-12-16T19:57:38Z

/botio test

pdfjsbot · 2014-12-16T19:57:38Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/0b989db6ebb4dd6/output.txt

pdfjsbot · 2014-12-16T19:57:38Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/8557598532920fb/output.txt

pdfjsbot · 2014-12-16T20:14:45Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/8557598532920fb/output.txt

Total script time: 17.11 mins

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

pdfjsbot · 2014-12-16T20:20:02Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/0b989db6ebb4dd6/output.txt

Total script time: 22.39 mins

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

yurydelendik · 2014-12-18T17:33:40Z

src/display/canvas.js


      var glyph = this.processingType3;

-      if (COMPILE_TYPE3_GLYPHS && glyph && !('compiled' in glyph)) {
-        var MAX_SIZE_TO_COMPILE = 1000;
+      if (COMPILE_TYPE3_GLYPHS && glyph && glyph['compiled'] === undefined) {


nit: glyph.compiled === undefined

i am fine with changing, but there are more places.

That's fine -- we can fix that latter. I'm looking at that in this code context. Thanks

yurydelendik · 2014-12-18T17:34:46Z

Good to go with comment above fixed. Thanks

yurydelendik · 2014-12-18T19:19:01Z

src/display/canvas.js

@@ -829,7 +829,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    },
    setDash: function CanvasGraphics_setDash(dashArray, dashPhase) {
      var ctx = this.ctx;
-      if ('setLineDash' in ctx) {
+      if (ctx['setLineDash'] !== undefined) {


Oh, are you talking about these? Yeah, let's fix these too.

yurydelendik · 2014-12-18T20:05:20Z

/botio test

pdfjsbot · 2014-12-18T20:05:21Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/9f550d10b0c6b14/output.txt

pdfjsbot · 2014-12-18T20:05:21Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2014-12-18T20:22:53Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9f550d10b0c6b14/output.txt

Total script time: 17.52 mins

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

pdfjsbot · 2014-12-18T20:28:03Z

From: Bot.io (Linux)

Success

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

Total script time: 22.70 mins

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

yurydelendik · 2014-12-18T20:28:19Z

src/display/canvas.js

      destCtx.setLineDash(sourceCtx.getLineDash());
      destCtx.lineDashOffset =  sourceCtx.lineDashOffset;
-    } else if ('mozDash' in sourceCtx) {
+    } else if (sourceCtx.mozDash !== undefined) {


I remember mozDash property has to create an array, can we switch that to the mozDashOffset check?

minor improvements and code cleanup for canvas.js

yurydelendik · 2014-12-18T21:06:19Z

Excellent! Thank you.

…Canvasjs minor improvements and code cleanup for canvas.js

Snuffleupagus reviewed Oct 26, 2014
View reviewed changes

CodingFabian force-pushed the minorImprovementsInCanvasjs branch 2 times, most recently from 81fcf8f to 00b95d9 Compare October 26, 2014 17:47

Snuffleupagus added the core label Oct 26, 2014

CodingFabian force-pushed the minorImprovementsInCanvasjs branch from 00b95d9 to 6869697 Compare December 16, 2014 19:47

yurydelendik reviewed Dec 18, 2014
View reviewed changes

CodingFabian force-pushed the minorImprovementsInCanvasjs branch 2 times, most recently from d034a32 to c1b8e01 Compare December 18, 2014 19:15

yurydelendik reviewed Dec 18, 2014
View reviewed changes

CodingFabian force-pushed the minorImprovementsInCanvasjs branch from c1b8e01 to 6ffcafe Compare December 18, 2014 19:58

yurydelendik reviewed Dec 18, 2014
View reviewed changes

minor improvements and code cleanup for canvas.js

5405b1c

CodingFabian force-pushed the minorImprovementsInCanvasjs branch from 6ffcafe to 5405b1c Compare December 18, 2014 20:59

yurydelendik added a commit that referenced this pull request Dec 18, 2014

Merge pull request #5446 from CodingFabian/minorImprovementsInCanvasjs

a018e93

minor improvements and code cleanup for canvas.js

yurydelendik merged commit a018e93 into mozilla:master Dec 18, 2014

speedplane pushed a commit to speedplane/pdf.js that referenced this pull request Feb 24, 2015

Merge pull request mozilla#5446 from CodingFabian/minorImprovementsIn…

5e671ba

…Canvasjs minor improvements and code cleanup for canvas.js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minor improvements and code cleanup for canvas.js #5446

minor improvements and code cleanup for canvas.js #5446

CodingFabian commented Oct 26, 2014

Snuffleupagus Oct 26, 2014

CodingFabian Oct 26, 2014

timvandermeij commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

CodingFabian commented Dec 16, 2014

CodingFabian commented Dec 16, 2014

Snuffleupagus commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

yurydelendik Dec 18, 2014

CodingFabian Dec 18, 2014

yurydelendik Dec 18, 2014

yurydelendik commented Dec 18, 2014

yurydelendik Dec 18, 2014

CodingFabian Dec 18, 2014

yurydelendik commented Dec 18, 2014

pdfjsbot commented Dec 18, 2014

pdfjsbot commented Dec 18, 2014

pdfjsbot commented Dec 18, 2014

pdfjsbot commented Dec 18, 2014

yurydelendik Dec 18, 2014

CodingFabian Dec 18, 2014

yurydelendik commented Dec 18, 2014

minor improvements and code cleanup for canvas.js #5446

minor improvements and code cleanup for canvas.js #5446

Conversation

CodingFabian commented Oct 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 16, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 16, 2014

From: Bot.io (Windows)

Failed

pdfjsbot commented Dec 16, 2014

From: Bot.io (Linux)

Failed

CodingFabian commented Dec 16, 2014

CodingFabian commented Dec 16, 2014

Snuffleupagus commented Dec 16, 2014

pdfjsbot commented Dec 16, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 16, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 16, 2014

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 16, 2014

From: Bot.io (Linux)

Success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Dec 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Dec 18, 2014

pdfjsbot commented Dec 18, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 18, 2014

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 18, 2014

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 18, 2014

From: Bot.io (Linux)

Success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Dec 18, 2014