From 27b92a6007133e65450b53d6f7364f5a77087cf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Lehni?= Date: Wed, 3 Oct 2018 16:45:38 +0200 Subject: [PATCH] Improve handling of push() with lots of items Improves fix for #1493, should pass CI again --- src/core/Base.js | 25 +++++++++++++++++++++++-- src/node/canvas.js | 2 +- src/path/CompoundPath.js | 5 +++-- src/path/Curve.js | 2 +- src/path/Path.js | 9 +-------- src/path/PathItem.Boolean.js | 6 +++--- test/tests/Path.js | 10 +++++----- 7 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/core/Base.js b/src/core/Base.js index a5f60711..834c940f 100644 --- a/src/core/Base.js +++ b/src/core/Base.js @@ -571,6 +571,27 @@ statics: /** @lends Base */{ }); }, + /** + * Utility function for pushing a large amount of items to an array. + */ + push: function(list, items) { + var itemsLength = items.length; + // It seems for "small" amounts of items, this performs better, + // but once it reaches a certain amount, some browsers start crashing: + if (itemsLength < 4096) { + list.push.apply(list, items); + } else { + // Use a loop as the best way to handle big arrays (see #1493). + // Set new array length once before the loop for better performance. + var startLength = list.length; + list.length += itemsLength; + for (var i = 0; i < itemsLength; i++) { + list[startLength + i] = items[i]; + } + } + return list; + }, + /** * Utility function for adding and removing items from a list of which each * entry keeps a reference to its index in the list in the private _index @@ -587,14 +608,14 @@ statics: /** @lends Base */{ items[i]._index = index + i; if (append) { // Append them all at the end by using push - list.push.apply(list, items); + Base.push(list, items); // Nothing removed, and nothing to adjust above return []; } else { // Insert somewhere else and/or remove var args = [index, remove]; if (items) - args.push.apply(args, items); + Base.push(args, items); var removed = list.splice.apply(list, args); // Erase the indices of the removed items for (var i = 0, l = removed.length; i < l; i++) diff --git a/src/node/canvas.js b/src/node/canvas.js index d758643b..fcb96540 100644 --- a/src/node/canvas.js +++ b/src/node/canvas.js @@ -19,7 +19,7 @@ module.exports = function(self, requireName) { var Canvas; try { Canvas = require('canvas'); - } catch(e) { + } catch(error) { // Remove `self.window`, so we still have the global `self` reference, // but no `window` object: // - On the browser, this corresponds to a worker context. diff --git a/src/path/CompoundPath.js b/src/path/CompoundPath.js index e69a7f88..d1bedfd3 100644 --- a/src/path/CompoundPath.js +++ b/src/path/CompoundPath.js @@ -213,8 +213,9 @@ var CompoundPath = PathItem.extend(/** @lends CompoundPath# */{ getCurves: function() { var children = this._children, curves = []; - for (var i = 0, l = children.length; i < l; i++) - curves.push.apply(curves, children[i].getCurves()); + for (var i = 0, l = children.length; i < l; i++) { + Base.push(curves, children[i].getCurves()); + } return curves; }, diff --git a/src/path/Curve.js b/src/path/Curve.js index 83699a6a..c2baabdc 100644 --- a/src/path/Curve.js +++ b/src/path/Curve.js @@ -2128,7 +2128,7 @@ new function() { // Scope for bezier intersection using fat-line clipping // Flatten the list of location arrays to one array and return it. locations = []; for (var i = 0, l = arrays.length; i < l; i++) { - locations.push.apply(locations, arrays[i]); + Base.push(locations, arrays[i]); } return locations; } diff --git a/src/path/Path.js b/src/path/Path.js index 2dca56fe..a4d02ca2 100644 --- a/src/path/Path.js +++ b/src/path/Path.js @@ -405,14 +405,7 @@ var Path = PathItem.extend(/** @lends Path# */{ } if (append) { // Append them all at the end. - // Use a loop as the best way to handle big arrays (see #1493). - // Set future array length before the loop for better performances. - var originalLength = segments.length; - var offsetLength = segs.length; - segments.length += offsetLength; - for (var i = 0; i < offsetLength; i++) { - segments[originalLength + i] = segs[i]; - } + Base.push(segments, segs); } else { // Insert somewhere else segments.splice.apply(segments, [index, 0].concat(segs)); diff --git a/src/path/PathItem.Boolean.js b/src/path/PathItem.Boolean.js index 0d6e1c5f..49b129fd 100644 --- a/src/path/PathItem.Boolean.js +++ b/src/path/PathItem.Boolean.js @@ -112,8 +112,8 @@ PathItem.inject(new function() { function collect(paths) { for (var i = 0, l = paths.length; i < l; i++) { var path = paths[i]; - segments.push.apply(segments, path._segments); - curves.push.apply(curves, path.getCurves()); + Base.push(segments, path._segments); + Base.push(curves, path.getCurves()); // See if all encountered segments in a path are overlaps, to // be able to separately handle fully overlapping paths. path._overlapsOnly = true; @@ -1236,7 +1236,7 @@ PathItem.inject(new function() { clearCurveHandles(clearCurves); // Finally resolve self-intersections through tracePaths() paths = tracePaths(Base.each(paths, function(path) { - this.push.apply(this, path._segments); + Base.push(this, path._segments); }, [])); } // Determine how to return the paths: First try to recycle the diff --git a/test/tests/Path.js b/test/tests/Path.js index ae190b10..f198d17d 100644 --- a/test/tests/Path.js +++ b/test/tests/Path.js @@ -249,7 +249,6 @@ test('After removing all segments of a selected path, it should still be selecte }, true); }); - test('After simplifying a path using #simplify(), the path should stay fullySelected', function() { var path = new Path(); for (var i = 0; i < 30; i++) { @@ -612,11 +611,12 @@ test('Path#arcTo(from, through, to); where from, through and to all share the sa equals(error != null, true, 'We expect this arcTo() command to throw an error'); }); -test('#1493 Path#add with 1000000 segments', function() { - var path = new Path(); - for (var i = 0; i < 1000000; i++) { - path.add(new Point(0, 0)); +test('Path#add() with a lot of segments (#1493)', function() { + var segments = []; + for (var i = 0; i < 100000; i++) { + segments.push(new Point(0, 0)); } + var path = new Path(segments); path.clone(); expect(0); });