From 40e69d39ebd4ee0f28918d58c738c9f19eaf4a2c Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Fri, 31 Mar 2017 16:56:12 -0400 Subject: [PATCH 01/12] Optimize performance of setCategoryIndex I added an auxiliary map for ``ax._categories`` array to avoid using ``Array.prototype.indexOf`` function searching for the existence of a category. I tested [here](https://jsfiddle.net/smileyhaowen/cvLzxz7L/2/) and observed ~20% performance improvements on big amount of traces. --- src/plots/cartesian/set_convert.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 903864504e8..33aa1822f75 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -126,12 +126,16 @@ module.exports = function setConvert(ax, fullLayout) { */ function setCategoryIndex(v) { if(v !== null && v !== undefined) { - var c = ax._categories.indexOf(v); - if(c === -1) { + + if(ax._categoriesMap[v] === undefined) { ax._categories.push(v); - return ax._categories.length - 1; + + var curLength = ax._categories.length - 1; + ax._categoriesMap[v] = curLength; + + return curLength; } - return c; + return ax._categoriesMap[v]; } return BADNUM; } @@ -325,6 +329,8 @@ module.exports = function setConvert(ax, fullLayout) { // TODO cleaner way to handle this case if(!ax._categories) ax._categories = []; + // Add a map to optimize the performance of category collection + if(!ax._categoriesMap) ax._categoriesMap = {}; // make sure we have a domain (pull it in from the axis // this one is overlaying if necessary) From b47c7afb6bf851cf2ddb87f34436c210b8cf7051 Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Fri, 31 Mar 2017 17:00:07 -0400 Subject: [PATCH 02/12] Update unit test --- test/jasmine/tests/axes_test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/jasmine/tests/axes_test.js b/test/jasmine/tests/axes_test.js index 6f82ef2b067..38590704bec 100644 --- a/test/jasmine/tests/axes_test.js +++ b/test/jasmine/tests/axes_test.js @@ -1804,6 +1804,7 @@ describe('Test axes', function() { function _autoBin(x, ax, nbins) { ax._categories = []; + ax._categoriesMap = {}; Axes.setConvert(ax); var d = ax.makeCalcdata({ x: x }, 'x'); From e8941d6367dd138e54d9c89aecd8caf824c1af16 Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Mon, 3 Apr 2017 18:45:23 -0400 Subject: [PATCH 03/12] Refactor the changes --- src/plots/cartesian/set_convert.js | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 33aa1822f75..76afc687cf1 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -126,8 +126,12 @@ module.exports = function setConvert(ax, fullLayout) { */ function setCategoryIndex(v) { if(v !== null && v !== undefined) { + if (ax._categoriesMap === undefined) + ax._categoriesMap = {}; - if(ax._categoriesMap[v] === undefined) { + if (ax._categoriesMap[v] !== undefined) { + return ax._categoriesMap[v]; + } else { ax._categories.push(v); var curLength = ax._categories.length - 1; @@ -135,7 +139,6 @@ module.exports = function setConvert(ax, fullLayout) { return curLength; } - return ax._categoriesMap[v]; } return BADNUM; } @@ -143,8 +146,10 @@ module.exports = function setConvert(ax, fullLayout) { function getCategoryIndex(v) { // d2l/d2c variant that that won't add categories but will also // allow numbers to be mapped to the linearized axis positions - var index = ax._categories.indexOf(v); - if(index !== -1) return index; + if(ax._categoriesMap) + var index = ax._categoriesMap[v]?ax._categoriesMap:undefined; + + if(index !== undefined) return index; if(typeof v === 'number') return v; } From b3fb03d45b8b46fb81e0e7c6e8530d50d8f1f83e Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Mon, 3 Apr 2017 18:49:38 -0400 Subject: [PATCH 04/12] Build the lookup map for initialized categories --- src/plots/plots.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/plots/plots.js b/src/plots/plots.js index 6f4dfcf913e..4c674d0c6ce 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1950,6 +1950,13 @@ plots.doCalcdata = function(gd, traces) { // to be filled in later by ax.d2c for(i = 0; i < axList.length; i++) { axList[i]._categories = axList[i]._initialCategories.slice(); + + //Build the lookup map for initialized categories + axList[i]._categoriesMap = {}; + for (var j = 0; j < axList[i]._categories.length; j++) { + axList[i]._categoriesMap[axList[i]._categories[j]] = j; + } + if(axList[i].type === 'category') hasCategoryAxis = true; } From 2992126c335b2182e00f6b69732ae0edb6a020d7 Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Mon, 3 Apr 2017 19:16:10 -0400 Subject: [PATCH 05/12] Reset the lookup map when the categories are reset --- src/plots/plots.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/plots/plots.js b/src/plots/plots.js index 4c674d0c6ce..ccef6b73946 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2001,6 +2001,8 @@ plots.doCalcdata = function(gd, traces) { axList[i]._min = []; axList[i]._max = []; axList[i]._categories = []; + //Reset the look up map + axList[i]._categoriesMap = {}; } } From 989f07a67091733f9b1621de48fee6ea6660142c Mon Sep 17 00:00:00 2001 From: hy9be_uva Date: Wed, 5 Apr 2017 13:07:04 -0400 Subject: [PATCH 06/12] Optimize the scatter trace sorting comparator with map Construct a map rather than an array to improve the performance of index search. Jasmine tests passed. --- src/traces/scatter/plot.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index 7da03ffe2fe..8e367807a70 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -51,13 +51,13 @@ module.exports = function plot(gd, plotinfo, cdscatter, transitionOpts, makeOnCo // Sort the traces, once created, so that the ordering is preserved even when traces // are shown and hidden. This is needed since we're not just wiping everything out // and recreating on every update. - for(i = 0, uids = []; i < cdscatter.length; i++) { - uids[i] = cdscatter[i][0].trace.uid; + for(i = 0, uids = {}; i < cdscatter.length; i++) { + uids[cdscatter[i][0].trace.uid] = i; } scatterlayer.selectAll('g.trace').sort(function(a, b) { - var idx1 = uids.indexOf(a[0].trace.uid); - var idx2 = uids.indexOf(b[0].trace.uid); + var idx1 = uids[a[0].trace.uid]; + var idx2 = uids[b[0].trace.uid]; return idx1 > idx2 ? 1 : -1; }); From abcd729299f735e37391306e7abb8fb2ace66b4b Mon Sep 17 00:00:00 2001 From: Haowen You Date: Sat, 8 Apr 2017 13:24:45 -0400 Subject: [PATCH 07/12] Clean ESLint errors in CI --- src/plots/cartesian/set_convert.js | 14 +++++++------- src/plots/plots.js | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 76afc687cf1..5a3aacd61f1 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -126,10 +126,11 @@ module.exports = function setConvert(ax, fullLayout) { */ function setCategoryIndex(v) { if(v !== null && v !== undefined) { - if (ax._categoriesMap === undefined) + if(ax._categoriesMap === undefined) { ax._categoriesMap = {}; + } - if (ax._categoriesMap[v] !== undefined) { + if(ax._categoriesMap[v] !== undefined) { return ax._categoriesMap[v]; } else { ax._categories.push(v); @@ -146,11 +147,10 @@ module.exports = function setConvert(ax, fullLayout) { function getCategoryIndex(v) { // d2l/d2c variant that that won't add categories but will also // allow numbers to be mapped to the linearized axis positions - if(ax._categoriesMap) - var index = ax._categoriesMap[v]?ax._categoriesMap:undefined; - - if(index !== undefined) return index; - if(typeof v === 'number') return v; + var index; + if(ax._categoriesMap) { index = ax._categoriesMap[v] ? ax._categoriesMap : undefined; } + if(index !== undefined) { return index; } + if(typeof v === 'number') { return v; } } function l2p(v) { diff --git a/src/plots/plots.js b/src/plots/plots.js index ccef6b73946..d84500bc07b 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1951,9 +1951,9 @@ plots.doCalcdata = function(gd, traces) { for(i = 0; i < axList.length; i++) { axList[i]._categories = axList[i]._initialCategories.slice(); - //Build the lookup map for initialized categories + // Build the lookup map for initialized categories axList[i]._categoriesMap = {}; - for (var j = 0; j < axList[i]._categories.length; j++) { + for(j = 0; j < axList[i]._categories.length; j++) { axList[i]._categoriesMap[axList[i]._categories[j]] = j; } @@ -2001,7 +2001,7 @@ plots.doCalcdata = function(gd, traces) { axList[i]._min = []; axList[i]._max = []; axList[i]._categories = []; - //Reset the look up map + // Reset the look up map axList[i]._categoriesMap = {}; } } From 153f547810a48c408fa32a08808ba6bef40e075b Mon Sep 17 00:00:00 2001 From: Haowen You Date: Sat, 8 Apr 2017 14:57:51 -0400 Subject: [PATCH 08/12] getCategoryIndex falls back to the former implementation to fix the failed unit test case --- src/plots/cartesian/set_convert.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 5a3aacd61f1..76599ec38d6 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -148,8 +148,13 @@ module.exports = function setConvert(ax, fullLayout) { // d2l/d2c variant that that won't add categories but will also // allow numbers to be mapped to the linearized axis positions var index; - if(ax._categoriesMap) { index = ax._categoriesMap[v] ? ax._categoriesMap : undefined; } - if(index !== undefined) { return index; } + if(ax._categoriesMap) { + index = ax._categoriesMap[v] ? ax._categoriesMap : undefined; + if(index !== undefined) return index; + } else { + index = ax._categories.indexOf(v); + if(index !== -1) return index; + } if(typeof v === 'number') { return v; } } From ff043b458a6e6d95dc26399bc9f6a92486571880 Mon Sep 17 00:00:00 2001 From: Haowen You Date: Sat, 8 Apr 2017 15:20:19 -0400 Subject: [PATCH 09/12] Fix another problem in getCategoryIndex --- src/plots/cartesian/set_convert.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 76599ec38d6..8f9e97f43e6 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -149,7 +149,7 @@ module.exports = function setConvert(ax, fullLayout) { // allow numbers to be mapped to the linearized axis positions var index; if(ax._categoriesMap) { - index = ax._categoriesMap[v] ? ax._categoriesMap : undefined; + index = ax._categoriesMap[v]; if(index !== undefined) return index; } else { index = ax._categories.indexOf(v); From c32163e63babdea0fe1d018dcb5254e6279f1bd0 Mon Sep 17 00:00:00 2001 From: Haowen You Date: Sat, 8 Apr 2017 16:05:20 -0400 Subject: [PATCH 10/12] Add comments --- src/plots/cartesian/set_convert.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 8f9e97f43e6..109f05cb76f 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -148,6 +148,7 @@ module.exports = function setConvert(ax, fullLayout) { // d2l/d2c variant that that won't add categories but will also // allow numbers to be mapped to the linearized axis positions var index; + // Get the index from the aux map if it was built, otherwise search the _categories array if(ax._categoriesMap) { index = ax._categoriesMap[v]; if(index !== undefined) return index; From d2be6c94f3291c42666bf5c3f086968f42010ba2 Mon Sep 17 00:00:00 2001 From: hy9be Date: Mon, 10 Apr 2017 12:35:08 -0400 Subject: [PATCH 11/12] Remove the fallback to indexOf search in getCategoryIndex, update the mock object in unit test instead --- src/plots/cartesian/set_convert.js | 4 +--- test/jasmine/tests/axes_test.js | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 109f05cb76f..823562f5854 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -152,10 +152,8 @@ module.exports = function setConvert(ax, fullLayout) { if(ax._categoriesMap) { index = ax._categoriesMap[v]; if(index !== undefined) return index; - } else { - index = ax._categories.indexOf(v); - if(index !== -1) return index; } + if(typeof v === 'number') { return v; } } diff --git a/test/jasmine/tests/axes_test.js b/test/jasmine/tests/axes_test.js index 38590704bec..73c04444b2e 100644 --- a/test/jasmine/tests/axes_test.js +++ b/test/jasmine/tests/axes_test.js @@ -1685,6 +1685,7 @@ describe('Test axes', function() { var ax = { type: 'category', _categories: ['a', 'b', 'c', 'd'], + _categoriesMap: {'a': 0, 'b': 1, 'c': 2, 'd': 3}, tickmode: 'array', tickvals: ['a', 1, 1.5, 'c', 2.7, 3, 'e', 4, 5, -2], ticktext: ['A!', 'B?', 'B->C'], From 14d66495c2f3f889856ae0db7513012904835038 Mon Sep 17 00:00:00 2001 From: hy9be Date: Mon, 10 Apr 2017 15:41:28 -0400 Subject: [PATCH 12/12] Remove out of date comments --- src/plots/cartesian/set_convert.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/plots/cartesian/set_convert.js b/src/plots/cartesian/set_convert.js index 823562f5854..2d7c26cc3f2 100644 --- a/src/plots/cartesian/set_convert.js +++ b/src/plots/cartesian/set_convert.js @@ -147,10 +147,8 @@ module.exports = function setConvert(ax, fullLayout) { function getCategoryIndex(v) { // d2l/d2c variant that that won't add categories but will also // allow numbers to be mapped to the linearized axis positions - var index; - // Get the index from the aux map if it was built, otherwise search the _categories array if(ax._categoriesMap) { - index = ax._categoriesMap[v]; + var index = ax._categoriesMap[v]; if(index !== undefined) return index; }