Colorspace: refactoring to prevent unnecessary creation of intermediate arrays #7863

timvandermeij · 2016-12-01T23:47:43Z

This commit series refactors the colorspace code to prevent the creation of unnecessary intermediate arrays and to make the code shorter and more readable. The commit messages contain more information about the changes.

Reviewing is slightly easier with https://github.com/mozilla/pdf.js/pull/7863/files?w=1.

Snuffleupagus

Really nice that you've decided to try and make colorspace.js more efficient!

I'm including a benchmark of the changes, [...], each with 6 runs.

The jsPerf results certainly seem convincing, but I've got one question here.
I'm well aware that those files are slow to render, but unfortunately 6 runs is way to low to be able to say with any certainty whether a change really improves performance (since statistically the sample size is way too low, an individual test run could actually skew the final results).
Would you be able to run the tests with at least 25 (or even 50) runs each?

Snuffleupagus · 2016-12-02T10:42:11Z

src/core/colorspace.js

@@ -294,13 +294,10 @@ var ColorSpace = (function ColorSpaceClosure() {
    }

    cs = xref.fetchIfRef(cs);
-    var mode;


I cannot help wondering if it isn't more efficient to use a local variable, rather than adding something to this that will not actually be used outside of this particular method?
Couldn't this actually increase memory usage, albeit a very tiny bit, since you're now (unnecessarily) storing more state?
Have you benchmarked this particular change to see if it really helps?

Fixed in the new commit. Now that you mentioned this, I also noticed that it's not used outside of this method, so it may just as well be only local.

Snuffleupagus · 2016-12-02T10:49:42Z

src/core/colorspace.js

@@ -606,7 +606,7 @@ var DeviceGrayCS = (function DeviceGrayCSClosure() {
    getRgbItem: function DeviceGrayCS_getRgbItem(src, srcOffset,
                                                 dest, destOffset) {
      var c = (src[srcOffset] * 255) | 0;
-      c = c < 0 ? 0 : c > 255 ? 255 : c;
+      c = Math.min(Math.max(c, 0), 255);


Would it make sense, as in would it help the JS engine to optimize these computations even further, if this code was moved to e.g. a clamp function placed at the top of this file (or even a utility function to be used in more files)?

Also, I noticed that in LabCS clamping is still done the old way.

It's been awhile since I looked at this, but using an inline conditional use to always be faster. If I remember correctly, we actually removed calls to clamp an inlined the clamping.

Fixed in the new commit. I created a utility function for clamping and used that in all places for clarity. I fixed the clamping in LabCS as well. There is one last manual clamping part in LabCS, but since that uses a different kind of clamping (first to a [0,1] range and then to a [0,255] range in one step), I did not find a suitable way to convert that to use the clamp function and not break functionality, so I propose we leave that one instance as-is.

Snuffleupagus · 2016-12-02T10:55:10Z

src/core/colorspace.js

@@ -748,7 +748,8 @@ var DeviceCmykCS = (function DeviceCmykCSClosure() {
  function DeviceCmykCS() {
    this.name = 'DeviceCMYK';
    this.numComps = 4;
-    this.defaultColor = new Float32Array([0, 0, 0, 1]);
+    this.defaultColor = new Float32Array(this.numComps);
+    this.defaultColor[3] = 1;


Nit: maybe some sort of comment would be good here, since it might not be immediately clear why the "standard" this.defaultColor = new Float32Array(this.numComps); isn't enought in this particular case?

Fixed in the new commit.

This patch avoids the creation of extra arrays when initializing an array with default (zero) values. Doing this additionally makes the code more readable by allocating enough space for the number of color components.

- Remove an unnecessary check and assignment. - Clean up code regarding mode setting (no need for a member variable). - Indent two methods correctly.

brendandahl · 2016-12-02T19:16:27Z

Reposting...It's been awhile since I looked at this, but using an inline conditional use to always be faster. If I remember correctly, we actually removed calls to clamp an inlined the clamping.

timvandermeij · 2016-12-06T00:23:52Z

I removed the commit with the clamping code as its performance impact is hard to measure objectively and it's unclear if the additional function call is bad or not (even though the code is more readable). This can be done later as it would otherwise unnecessarily delay landing of the other commits.

@Snuffleupagus Would you be willing to review this patch as well? It's now only a clean-up PR that won't have a performance impact anymore (only slight memory usage reduction because of the arrays, but it's probably not even measurable).

timvandermeij · 2016-12-06T00:44:22Z

/botio-linux preview

pdfjsbot · 2016-12-06T00:44:23Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/7f2f58e7c247fb8/output.txt

pdfjsbot · 2016-12-06T00:46:30Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/7f2f58e7c247fb8/output.txt

Total script time: 2.12 mins

Published

timvandermeij · 2016-12-06T00:47:06Z

/botio test

pdfjsbot · 2016-12-06T00:47:06Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-12-06T00:47:06Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2016-12-06T01:13:17Z

From: Bot.io (Windows)

Success

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

Total script time: 26.17 mins

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

pdfjsbot · 2016-12-06T01:14:30Z

From: Bot.io (Linux)

Success

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

Total script time: 27.39 mins

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

Snuffleupagus

Seems fine to me, thanks for the clean-up!

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays

timvandermeij added core performance labels Dec 1, 2016

Snuffleupagus reviewed Dec 2, 2016

View reviewed changes

timvandermeij added 3 commits December 2, 2016 16:42

Colorspace: optimize default color initialization

ef653d9

This patch avoids the creation of extra arrays when initializing an array with default (zero) values. Doing this additionally makes the code more readable by allocating enough space for the number of color components.

Colorspace: reduce duplication in AlternateCS.getRgbBuffer

c5c0a00

Colorspace: miscellaneous improvements

90d9481

- Remove an unnecessary check and assignment. - Clean up code regarding mode setting (no need for a member variable). - Indent two methods correctly.

timvandermeij force-pushed the colorspace branch 2 times, most recently from 2508325 to 06fae58 Compare December 2, 2016 18:56

timvandermeij force-pushed the colorspace branch from 06fae58 to 90d9481 Compare December 6, 2016 00:14

timvandermeij changed the title ~~Colorspace: improve performance~~ Colorspace: refactoring to prevent unnecessary creation of intermediate arrays Dec 6, 2016

timvandermeij assigned Snuffleupagus Dec 6, 2016

timvandermeij removed the performance label Dec 6, 2016

Snuffleupagus approved these changes Dec 6, 2016

View reviewed changes

Snuffleupagus merged commit 94ddd8f into mozilla:master Dec 6, 2016

timvandermeij deleted the colorspace branch December 6, 2016 21:23

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7863 from timvandermeij/colorspace

e780c87

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays #7863

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays #7863

timvandermeij commented Dec 1, 2016 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Dec 2, 2016

timvandermeij Dec 2, 2016

Snuffleupagus Dec 2, 2016 •

edited

Loading

brendandahl Dec 2, 2016

timvandermeij Dec 2, 2016 •

edited

Loading

Snuffleupagus Dec 2, 2016

timvandermeij Dec 2, 2016

brendandahl commented Dec 2, 2016

timvandermeij commented Dec 6, 2016 •

edited

Loading

timvandermeij commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

timvandermeij commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

Snuffleupagus left a comment

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays #7863

Colorspace: refactoring to prevent unnecessary creation of intermediate arrays #7863

Conversation

timvandermeij commented Dec 1, 2016 • edited Loading

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Dec 2, 2016

Choose a reason for hiding this comment

timvandermeij Dec 2, 2016

Choose a reason for hiding this comment

Snuffleupagus Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

brendandahl Dec 2, 2016

Choose a reason for hiding this comment

timvandermeij Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Dec 2, 2016

Choose a reason for hiding this comment

timvandermeij Dec 2, 2016

Choose a reason for hiding this comment

brendandahl commented Dec 2, 2016

timvandermeij commented Dec 6, 2016 • edited Loading

timvandermeij commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 6, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Dec 6, 2016

pdfjsbot commented Dec 6, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Dec 6, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Dec 6, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Dec 6, 2016

From: Bot.io (Linux)

Success

Snuffleupagus left a comment

Choose a reason for hiding this comment

timvandermeij commented Dec 1, 2016 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Dec 2, 2016 •

edited

Loading

timvandermeij Dec 2, 2016 •

edited

Loading

timvandermeij commented Dec 6, 2016 •

edited

Loading