From 4931606b63c54a836fd16e4df6a1ae2e9f47615e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Dumas?= Date: Mon, 22 Aug 2016 20:29:12 +1000 Subject: [PATCH] refs #139, #138: Performance based changes - financetime instanceof, string constructor removal, remove Array.join * Adding financetime tests to ensure scale of null and undefined are handled gracefully * Removing String constructor, profiling appears quicker without * Removing usage of Array.join. Array.join is very slow in chrome --- src/plot/axisannotation.js | 12 +++---- src/plot/candlestick.js | 43 ++++++----------------- src/plot/macd.js | 2 +- src/plot/plot.js | 15 ++++++-- src/plot/supstance.js | 9 ++--- src/plot/trendline.js | 8 ++--- src/plot/volume.js | 2 +- src/scale/financetime.js | 2 +- src/svg/arrow.js | 27 +++++++------- test/spec/bundle/plot/plotSpec.js | 13 +++++++ test/spec/bundle/scale/financetimeSpec.js | 8 +++++ 11 files changed, 70 insertions(+), 71 deletions(-) diff --git a/src/plot/axisannotation.js b/src/plot/axisannotation.js index 8bfca5e6..55016741 100644 --- a/src/plot/axisannotation.js +++ b/src/plot/axisannotation.js @@ -138,9 +138,9 @@ function backgroundPath(accessor, axis, height, width, point, neg) { if(height/2 < point) pt = height/2; else h = height/2-point; - return 'M 0 ' + value + ' l ' + String(neg*Math.max(axis.innerTickSize(), 1)) + ' ' + String(-pt) + - ' l 0 ' + String(-h) + ' l ' + (neg*width) + ' 0 l 0 ' + height + - ' l ' + (neg*-width) + ' 0 l 0 ' + String(-h); + return 'M 0 ' + value + ' l ' + (neg*Math.max(axis.innerTickSize(), 1)) + ' ' + (-pt) + + ' l 0 ' + (-h) + ' l ' + (neg*width) + ' 0 l 0 ' + height + + ' l ' + (neg*-width) + ' 0 l 0 ' + (-h); case 'top': case 'bottom': var w = 0; @@ -148,9 +148,9 @@ function backgroundPath(accessor, axis, height, width, point, neg) { if(width/2 < point) pt = width/2; else w = width/2-point; - return 'M ' + value + ' 0 l ' + String(-pt) + ' ' + (neg*Math.max(axis.innerTickSize(), 1)) + - ' l ' + String(-w) + ' 0 l 0 ' + String(neg*height) + ' l ' + width + ' 0 l 0 ' + (neg*-height) + - ' l ' + String(-w) + ' 0'; + return 'M ' + value + ' 0 l ' + (-pt) + ' ' + (neg*Math.max(axis.innerTickSize(), 1)) + + ' l ' + (-w) + ' 0 l 0 ' + (neg*height) + ' l ' + width + ' 0 l 0 ' + (neg*-height) + + ' l ' + (-w) + ' 0'; default: throw "Unsupported axis.orient() = " + axis.orient(); } }; diff --git a/src/plot/candlestick.js b/src/plot/candlestick.js index e817e358..a4eac0e2 100644 --- a/src/plot/candlestick.js +++ b/src/plot/candlestick.js @@ -35,26 +35,17 @@ module.exports = function(d3_scale_linear, d3_extent, accessor_ohlc, plot, plotM width = p.width(x); return function(d) { - var path = [], - open = y(accessor.o(d)), + var open = y(accessor.o(d)), close = y(accessor.c(d)), - xValue = x(accessor.d(d)) - width/2; - - path.push( - 'M', xValue, open, - 'l', width, 0 - ); + xValue = x(accessor.d(d)) - width/2, + path = 'M ' + xValue + ' ' + open + ' l ' + width + ' ' + 0; // Draw body only if there is a body (there is no stroke, so will not appear anyway) if(open != close) { - path.push( - 'L', xValue + width, close, - 'l', -width, 0, - 'L', xValue, open - ); + path += ' L ' + (xValue + width) + ' ' + close + ' l ' + -width + ' ' + 0 + ' L ' + xValue + ' ' + open; } - return path.join(' '); + return path; }; } @@ -65,32 +56,18 @@ module.exports = function(d3_scale_linear, d3_extent, accessor_ohlc, plot, plotM width = p.width(x); return function(d) { - var path = [], - open = y(accessor.o(d)), + var open = y(accessor.o(d)), close = y(accessor.c(d)), xPoint = x(accessor.d(d)), - xValue = xPoint - width/2; - - // Top - path.push( - 'M', xPoint, y(accessor.h(d)), - 'L', xPoint, Math.min(open, close) - ); + xValue = xPoint - width/2, + path = 'M ' + xPoint + ' ' + y(accessor.h(d)) +' L ' + xPoint + ' '+ Math.min(open, close); // Top // Draw another cross wick if there is no body if(open == close) { - path.push( - 'M', xValue, open, - 'l', width, 0 - ); + path += ' M ' + xValue + ' ' + open + ' l ' + width + ' ' + 0; } // Bottom - path.push( - 'M', xPoint, Math.max(open, close), - 'L', xPoint, y(accessor.l(d)) - ); - - return path.join(' '); + return path + ' M ' + xPoint + ' ' + Math.max(open, close) + ' L ' + xPoint + ' ' + y(accessor.l(d)); }; } diff --git a/src/plot/macd.js b/src/plot/macd.js index 0a9f43e1..36de62aa 100644 --- a/src/plot/macd.js +++ b/src/plot/macd.js @@ -40,7 +40,7 @@ module.exports = function(accessor_macd, plot, plotMixin) { // Injected depende xValue = x(accessor.d(d)) - width/2; return 'M ' + xValue + ' ' + zero + ' l 0 ' + height + ' l ' + width + - ' 0 l 0 ' + String(-height); + ' 0 l 0 ' + (-height); }; } diff --git a/src/plot/plot.js b/src/plot/plot.js index eca4a210..2a0c47ce 100644 --- a/src/plot/plot.js +++ b/src/plot/plot.js @@ -75,8 +75,8 @@ module.exports = function(d3_svg_line, d3_select) { } function appendPlotTypePath(g, data, plotNames, direction) { - g.selectAll('path.' + plotNames.join('.') + '.' + direction).data(function(d) { return [d.filter(data)]; }) - .enter().append('path').attr('class', plotNames.join(' ') + ' ' + direction); + g.selectAll('path.' + arrayJoin(plotNames, '.') + '.' + direction).data(function(d) { return [d.filter(data)]; }) + .enter().append('path').attr('class', arrayJoin(plotNames, ' ') + ' ' + direction); } function barWidth(x) { @@ -84,6 +84,15 @@ module.exports = function(d3_svg_line, d3_select) { else return 3; // If it's not a finance time, the user should specify the band calculation (or constant) on the plot } + function arrayJoin(array, delimiter) { + if(!array.length) return; + var result = array[0]; + for(var i = 1; i < array.length; i++) { + result += delimiter + array[i]; + } + return result; + } + return { dataMapper: { unity: function(d) { return d; }, @@ -139,7 +148,7 @@ module.exports = function(d3_svg_line, d3_select) { */ joinPath: function(path) { return function(data) { - return data.map(path()).join(' '); + return arrayJoin(data.map(path()), ' '); }; }, diff --git a/src/plot/supstance.js b/src/plot/supstance.js index 42b0425e..617f532f 100644 --- a/src/plot/supstance.js +++ b/src/plot/supstance.js @@ -74,12 +74,9 @@ function refresh(g, plot, accessor, x, y, annotationSelection, annotation) { function supstancePath(accessor, x, y) { return function(d) { - var path = [], - range = x.range(); + var range = x.range(); - path.push('M', range[0], y(accessor(d))); - path.push('L', range[range.length-1], y(accessor(d))); - - return path.join(' '); + return 'M ' + range[0] + ' ' + y(accessor(d)) + + ' L ' + range[range.length-1] + ' ' + y(accessor(d)); }; } \ No newline at end of file diff --git a/src/plot/trendline.js b/src/plot/trendline.js index 55554d29..ea4eefda 100644 --- a/src/plot/trendline.js +++ b/src/plot/trendline.js @@ -102,12 +102,8 @@ function refresh(g, accessor, x, y) { function trendlinePath(accessor, x, y) { return function(d) { - var path = []; - - path.push('M', x(accessor.sd(d)), y(accessor.sv(d))); - path.push('L', x(accessor.ed(d)), y(accessor.ev(d))); - - return path.join(' '); + return 'M ' + x(accessor.sd(d))+ ' ' + y(accessor.sv(d)) + + ' L ' + x(accessor.ed(d)) + ' ' + y(accessor.ev(d)); }; } diff --git a/src/plot/volume.js b/src/plot/volume.js index 9e9026e7..5a87f146 100644 --- a/src/plot/volume.js +++ b/src/plot/volume.js @@ -38,7 +38,7 @@ module.exports = function(accessor_volume, plot, plotMixin) { // Injected depen xValue = x(accessor.d(d)) - width/2; return 'M ' + xValue + ' ' + zero + ' l 0 ' + height + ' l ' + width + - ' 0 l 0 ' + String(-height); + ' 0 l 0 ' + (-height); }; } diff --git a/src/scale/financetime.js b/src/scale/financetime.js index e1f3e784..85862e15 100644 --- a/src/scale/financetime.js +++ b/src/scale/financetime.js @@ -34,7 +34,7 @@ module.exports = function(d3_scale_linear, d3_time, d3_bisect, techan_util_rebin * @returns {*} */ function scale(x, offset) { - var mappedIndex = dateIndexMap[x.getTime ? x.getTime() : +x]; + var mappedIndex = dateIndexMap[x instanceof Date ? x.getTime() : +x]; offset = offset || 0; // Make sure the value has been mapped, if not, determine if it's just before, round in, or just after domain diff --git a/src/svg/arrow.js b/src/svg/arrow.js index e8723065..b31ae51c 100644 --- a/src/svg/arrow.js +++ b/src/svg/arrow.js @@ -10,7 +10,7 @@ module.exports = function(d3_functor) { // Injected dependencies tail = d3_functor(true); function arrow(d, i) { - var path = [], + var path, x = fx(d, i), y = fy(d, i), w = width(d, i), @@ -22,32 +22,31 @@ module.exports = function(d3_functor) { // Injected dependencies pw = w/2, // Point width ph = t ? h/2 : h; // Point Height - path.push('M', x, y); + path = 'M ' + x + ' ' + y; switch(o) { case 'up': case 'down': - path.push('l', -pw, neg*ph, 'l', ws, 0); - if(t) path.push('l', 0, neg*ph); - path.push('l', ws, 0); - if(t) path.push('l', 0, -neg*ph); - path.push('l', ws, 0); + path += ' l ' + -pw + ' ' + neg*ph + ' l ' + ws + ' ' + 0; + if(t) path += ' l ' + 0 + ' ' + neg*ph; + path += ' l ' + ws + ' ' + 0; + if(t) path += ' l ' + 0 + ' ' + -neg*ph; + path += ' l ' + ws + ' ' + 0; break; case 'left': case 'right': - path.push('l', neg*ph, -pw, 'l', 0, ws); - if(t) path.push('l', neg*ph, 0); - path.push('l', 0, ws); - if(t) path.push('l', -neg*ph, 0); - path.push('l', 0, ws); + path += ' l ' + neg*ph + ' ' + -pw + ' l ' + 0 + ' ' + ws; + if(t) path += ' l ' + neg*ph + ' ' + 0; + path += ' l ' + 0 + ' ' + ws; + if(t) path += ' l ' + -neg*ph + ' ' + 0; + path += ' l ' + 0 + ' ' + ws; break; default: throw "Unsupported arrow.orient() = " + orient; } - path.push('z'); - return path.join(' '); + return path + ' z'; } arrow.x = function(_) { diff --git a/test/spec/bundle/plot/plotSpec.js b/test/spec/bundle/plot/plotSpec.js index d66246c5..ce436c45 100644 --- a/test/spec/bundle/plot/plotSpec.js +++ b/test/spec/bundle/plot/plotSpec.js @@ -19,6 +19,19 @@ techanModule('plot/plot', function(specBuilder) { x = techan.scale.financetime(); }); + describe('And I have a path generator that returns a path', function() { + function pathGenerator() { + return function(d) { + return 'L 123 M 1 Z'; + }; + } + + it('Then joinPath will join the paths together to a single string', function() { + // Don't care about values in the data array + expect(plot.joinPath(pathGenerator)([undefined, undefined, undefined])).toEqual('L 123 M 1 Z L 123 M 1 Z L 123 M 1 Z'); + }); + }); + describe('And scale has a small domain and large range', function() { beforeEach(function() { x.range([0, 1000]); diff --git a/test/spec/bundle/scale/financetimeSpec.js b/test/spec/bundle/scale/financetimeSpec.js index abfc78ad..5dbe33f0 100644 --- a/test/spec/bundle/scale/financetimeSpec.js +++ b/test/spec/bundle/scale/financetimeSpec.js @@ -37,6 +37,14 @@ techanModule('scale/financetime', function(specBuilder) { expect(financetime.band()).toEqual(80); }); + it('Then scale of undefined should not throw error and result in value just outside of the greater range extent (+undefined === NaN)', function() { + expect(financetime(undefined)).toEqual(1100); + }); + + it('Then scale of null should not throw error and result in value just outside of the lesser extent (+null === 0)', function() { + expect(financetime(null)).toEqual(0); + }); + it('Then scale of first index should return min widened range', function() { expect(Math.round(financetime(data[0])*1000)/1000).toEqual(100); });