From d6e0bde68b055244390624ee8eaa81afcb540463 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Fri, 10 Feb 2023 16:36:27 +0100 Subject: [PATCH 1/2] Support pushing large number of items in tree iterators - Provide utility to push in a callstack-safe manner - Add tests Fixes https://github.com/eclipse-theia/theia/issues/12171 --- .../core/src/browser/tree/tree-iterator.ts | 13 +++---- .../browser/tree/tree-selection-state.spec.ts | 33 +++++++++++++++-- packages/core/src/common/array-utils.spec.ts | 36 +++++++++++++++++++ packages/core/src/common/array-utils.ts | 29 +++++++++++++++ 4 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 packages/core/src/common/array-utils.spec.ts diff --git a/packages/core/src/browser/tree/tree-iterator.ts b/packages/core/src/browser/tree/tree-iterator.ts index b09b4099521d7..89458e15fcdfe 100644 --- a/packages/core/src/browser/tree/tree-iterator.ts +++ b/packages/core/src/browser/tree/tree-iterator.ts @@ -1,5 +1,5 @@ // ***************************************************************************** -// Copyright (C) 2017 TypeFox and others. +// Copyright (C) 2017-2023 TypeFox and others. // // This program and the accompanying materials are made available under the // terms of the Eclipse Public License v. 2.0 which is available at @@ -14,7 +14,8 @@ // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 // ***************************************************************************** -import { TreeNode, CompositeTreeNode } from './tree'; +import { ArrayUtils } from '../../common'; +import { CompositeTreeNode, TreeNode } from './tree'; import { ExpandableTreeNode } from './tree-expansion'; export interface TreeIterator extends Iterator { @@ -202,12 +203,12 @@ export namespace Iterators { * Generator for depth first, pre-order tree traversal iteration. */ export function* depthFirst(root: T, children: (node: T) => T[] | undefined, include: (node: T) => boolean = () => true): IterableIterator { - const stack: T[] = []; + let stack: T[] = []; stack.push(root); while (stack.length > 0) { const top = stack.pop()!; yield top; - stack.push(...(children(top) || []).filter(include).reverse()); + stack = ArrayUtils.pushAll(stack, (children(top) || []).filter(include).reverse()); } } @@ -215,12 +216,12 @@ export namespace Iterators { * Generator for breadth first tree traversal iteration. */ export function* breadthFirst(root: T, children: (node: T) => T[] | undefined, include: (node: T) => boolean = () => true): IterableIterator { - const queue: T[] = []; + let queue: T[] = []; queue.push(root); while (queue.length > 0) { const head = queue.shift()!; yield head; - queue.push(...(children(head) || []).filter(include)); + queue = ArrayUtils.pushAll(queue, (children(head) || []).filter(include)); } } diff --git a/packages/core/src/browser/tree/tree-selection-state.spec.ts b/packages/core/src/browser/tree/tree-selection-state.spec.ts index 52ae927602433..4e5974f1ab52b 100644 --- a/packages/core/src/browser/tree/tree-selection-state.spec.ts +++ b/packages/core/src/browser/tree/tree-selection-state.spec.ts @@ -1,5 +1,5 @@ // ***************************************************************************** -// Copyright (C) 2018 TypeFox and others. +// Copyright (C) 2018-2023 TypeFox and others. // // This program and the accompanying materials are made available under the // terms of the Eclipse Public License v. 2.0 which is available at @@ -16,10 +16,10 @@ import { expect } from 'chai'; import { MockTreeModel } from './test/mock-tree-model'; -import { TreeSelectionState } from './tree-selection-state'; import { createTreeTestContainer } from './test/tree-test-container'; -import { SelectableTreeNode, TreeSelection } from './tree-selection'; import { TreeModel } from './tree-model'; +import { SelectableTreeNode, TreeSelection } from './tree-selection'; +import { TreeSelectionState } from './tree-selection-state'; namespace TreeSelectionState { @@ -34,6 +34,16 @@ namespace TreeSelectionState { } +const LARGE_FLAT_MOCK_ROOT = (length = 250000) => { + const children = Array.from({ length }, (_, idx) => ({ 'id': (idx + 1).toString() })); + return MockTreeModel.Node.toTreeNode({ + 'id': 'ROOT', + 'children': [ + ...children + ] + }); +}; + describe('tree-selection-state', () => { const model = createTreeModel(); @@ -391,6 +401,23 @@ describe('tree-selection-state', () => { }); }); + it('should be able to handle range selection on large tree', () => { + model.root = LARGE_FLAT_MOCK_ROOT(); + expect(model.selectedNodes).to.be.empty; + + const start = 10; + const end = 20; + newState() + .nextState('toggle', start.toString(), { + focus: start.toString(), + selection: [start.toString()] + }) + .nextState('range', end.toString(), { + focus: start.toString(), + selection: Array.from({ length: end - start + 1 }, (_, idx) => (start + idx).toString()) + }); + }); + function newState(): TreeSelectionState.Assert { return nextState(new TreeSelectionState(model)); } diff --git a/packages/core/src/common/array-utils.spec.ts b/packages/core/src/common/array-utils.spec.ts new file mode 100644 index 0000000000000..8fa79b689cc93 --- /dev/null +++ b/packages/core/src/common/array-utils.spec.ts @@ -0,0 +1,36 @@ +// ***************************************************************************** +// Copyright (C) 2023 EclipseSource and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// http://www.eclipse.org/legal/epl-2.0. +// +// This Source Code may also be made available under the following Secondary +// Licenses when the conditions for such availability set forth in the Eclipse +// Public License v. 2.0 are satisfied: GNU General Public License, version 2 +// with the GNU Classpath Exception which is available at +// https://www.gnu.org/software/classpath/license.html. +// +// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 +// ***************************************************************************** + +import { expect } from 'chai'; +import { ArrayUtils } from './array-utils'; + +describe('array-utils', () => { + it('pushAll should allow pushing of large number of items', () => { + const initial = Array.from({ length: 10 }); + const addition = Array.from({ length: 1000000 }); + + const result = ArrayUtils.pushAll(initial, addition); + expect(result.length).to.equal(1000010); + }); + + it('pushAll should allow pushing of large number of items, independent from order', () => { + const initial = Array.from({ length: 1000000 }); + const addition = Array.from({ length: 10 }); + + const result = ArrayUtils.pushAll(initial, addition); + expect(result.length).to.equal(1000010); + }); +}); diff --git a/packages/core/src/common/array-utils.ts b/packages/core/src/common/array-utils.ts index 259b3822bc7c9..6fceea78a4a9a 100644 --- a/packages/core/src/common/array-utils.ts +++ b/packages/core/src/common/array-utils.ts @@ -106,4 +106,33 @@ export namespace ArrayUtils { export function coalesce(array: ReadonlyArray): T[] { return array.filter(e => !!e); } + + /** + * A safe variant to push additional items to an array. By default, the + * array push operation in combination with the spread operator is used. + * However, if the callstack size is exceeded on large additions we use the + * concatenation of arrays instead as it not depend on the callstack + * size. + * + * The original array might be modified. + * + * @param array An array of elements. + * @param items Additional elements to be added to the array. + * @returns An array containing the original elements with the additional + * elements appended. This may or may not be the array that was handed in. + */ + export function pushAll(array: T[], items: T[]): T[] { + try { + // typically faster but might fail depending on the number of items and the callstack size + array.push(...items); + return array; + } catch (error) { + if (error instanceof RangeError) { + // typically slower but works if we otherwise exceed the callstack size + // according to online benchmarks concat is faster than a forEach push + return array.concat(items); + } + throw error; + } + } } From a5ca4e4ae7b0721c6fa220b8dacb166b6b863195 Mon Sep 17 00:00:00 2001 From: Martin Fleck Date: Tue, 21 Feb 2023 11:42:05 +0100 Subject: [PATCH 2/2] Simplify by just removing spread operator --- .../core/src/browser/tree/tree-iterator.ts | 9 +++-- .../browser/tree/tree-selection-state.spec.ts | 2 +- packages/core/src/common/array-utils.spec.ts | 36 ------------------- packages/core/src/common/array-utils.ts | 29 --------------- 4 files changed, 5 insertions(+), 71 deletions(-) delete mode 100644 packages/core/src/common/array-utils.spec.ts diff --git a/packages/core/src/browser/tree/tree-iterator.ts b/packages/core/src/browser/tree/tree-iterator.ts index 89458e15fcdfe..fcc6074fee2f6 100644 --- a/packages/core/src/browser/tree/tree-iterator.ts +++ b/packages/core/src/browser/tree/tree-iterator.ts @@ -1,5 +1,5 @@ // ***************************************************************************** -// Copyright (C) 2017-2023 TypeFox and others. +// Copyright (C) 2017 TypeFox and others. // // This program and the accompanying materials are made available under the // terms of the Eclipse Public License v. 2.0 which is available at @@ -14,8 +14,7 @@ // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 // ***************************************************************************** -import { ArrayUtils } from '../../common'; -import { CompositeTreeNode, TreeNode } from './tree'; +import { TreeNode, CompositeTreeNode } from './tree'; import { ExpandableTreeNode } from './tree-expansion'; export interface TreeIterator extends Iterator { @@ -208,7 +207,7 @@ export namespace Iterators { while (stack.length > 0) { const top = stack.pop()!; yield top; - stack = ArrayUtils.pushAll(stack, (children(top) || []).filter(include).reverse()); + stack = stack.concat((children(top) || []).filter(include).reverse()); } } @@ -221,7 +220,7 @@ export namespace Iterators { while (queue.length > 0) { const head = queue.shift()!; yield head; - queue = ArrayUtils.pushAll(queue, (children(head) || []).filter(include)); + queue = queue.concat((children(head) || []).filter(include)); } } diff --git a/packages/core/src/browser/tree/tree-selection-state.spec.ts b/packages/core/src/browser/tree/tree-selection-state.spec.ts index 4e5974f1ab52b..cc82c6c9110c6 100644 --- a/packages/core/src/browser/tree/tree-selection-state.spec.ts +++ b/packages/core/src/browser/tree/tree-selection-state.spec.ts @@ -1,5 +1,5 @@ // ***************************************************************************** -// Copyright (C) 2018-2023 TypeFox and others. +// Copyright (C) 2018 TypeFox and others. // // This program and the accompanying materials are made available under the // terms of the Eclipse Public License v. 2.0 which is available at diff --git a/packages/core/src/common/array-utils.spec.ts b/packages/core/src/common/array-utils.spec.ts deleted file mode 100644 index 8fa79b689cc93..0000000000000 --- a/packages/core/src/common/array-utils.spec.ts +++ /dev/null @@ -1,36 +0,0 @@ -// ***************************************************************************** -// Copyright (C) 2023 EclipseSource and others. -// -// This program and the accompanying materials are made available under the -// terms of the Eclipse Public License v. 2.0 which is available at -// http://www.eclipse.org/legal/epl-2.0. -// -// This Source Code may also be made available under the following Secondary -// Licenses when the conditions for such availability set forth in the Eclipse -// Public License v. 2.0 are satisfied: GNU General Public License, version 2 -// with the GNU Classpath Exception which is available at -// https://www.gnu.org/software/classpath/license.html. -// -// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 -// ***************************************************************************** - -import { expect } from 'chai'; -import { ArrayUtils } from './array-utils'; - -describe('array-utils', () => { - it('pushAll should allow pushing of large number of items', () => { - const initial = Array.from({ length: 10 }); - const addition = Array.from({ length: 1000000 }); - - const result = ArrayUtils.pushAll(initial, addition); - expect(result.length).to.equal(1000010); - }); - - it('pushAll should allow pushing of large number of items, independent from order', () => { - const initial = Array.from({ length: 1000000 }); - const addition = Array.from({ length: 10 }); - - const result = ArrayUtils.pushAll(initial, addition); - expect(result.length).to.equal(1000010); - }); -}); diff --git a/packages/core/src/common/array-utils.ts b/packages/core/src/common/array-utils.ts index 6fceea78a4a9a..259b3822bc7c9 100644 --- a/packages/core/src/common/array-utils.ts +++ b/packages/core/src/common/array-utils.ts @@ -106,33 +106,4 @@ export namespace ArrayUtils { export function coalesce(array: ReadonlyArray): T[] { return array.filter(e => !!e); } - - /** - * A safe variant to push additional items to an array. By default, the - * array push operation in combination with the spread operator is used. - * However, if the callstack size is exceeded on large additions we use the - * concatenation of arrays instead as it not depend on the callstack - * size. - * - * The original array might be modified. - * - * @param array An array of elements. - * @param items Additional elements to be added to the array. - * @returns An array containing the original elements with the additional - * elements appended. This may or may not be the array that was handed in. - */ - export function pushAll(array: T[], items: T[]): T[] { - try { - // typically faster but might fail depending on the number of items and the callstack size - array.push(...items); - return array; - } catch (error) { - if (error instanceof RangeError) { - // typically slower but works if we otherwise exceed the callstack size - // according to online benchmarks concat is faster than a forEach push - return array.concat(items); - } - throw error; - } - } }