Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Maximum call stack size exceeded (#4694) #6716

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/compiler/compile/css/Stylesheet.ts
@@ -1,3 +1,4 @@
import { push_array } from '../../utils/push_array';
import MagicString from 'magic-string';
import { walk } from 'estree-walker';
import Selector from './Selector';
Expand Down Expand Up @@ -351,7 +352,7 @@ export default class Stylesheet {
const at_rule_declarations = node.block.children
.filter(node => node.type === 'Declaration')
.map(node => new Declaration(node));
atrule.declarations.push(...at_rule_declarations);
push_array(atrule.declarations, at_rule_declarations);
}

current_atrule = atrule;
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/compile/nodes/shared/map_children.ts
@@ -1,3 +1,4 @@
import { push_array } from '../../../utils/push_array';
import AwaitBlock from '../AwaitBlock';
import Body from '../Body';
import Comment from '../Comment';
Expand Down Expand Up @@ -58,7 +59,7 @@ export default function map_children(component, parent, scope, children: Templat
if (use_ignores) component.pop_ignores(), ignores = [];

if (node.type === 'Comment' && node.ignores.length) {
ignores.push(...node.ignores);
push_array(ignores, node.ignores);
}

if (last) last.next = node;
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/compile/render_dom/wrappers/Element/index.ts
@@ -1,3 +1,4 @@
import { push_array } from '../../../../utils/push_array';
import Renderer from '../../Renderer';
import Element from '../../../nodes/Element';
import Wrapper from '../shared/Wrapper';
Expand Down Expand Up @@ -596,7 +597,7 @@ export default class ElementWrapper extends Wrapper {
this.attributes.forEach((attribute) => {
if (attribute.node.name === 'class') {
const dependencies = attribute.node.get_dependencies();
this.class_dependencies.push(...dependencies);
push_array(this.class_dependencies, dependencies);
}
});

Expand Down
3 changes: 2 additions & 1 deletion src/compiler/compile/render_dom/wrappers/IfBlock.ts
@@ -1,3 +1,4 @@
import { push_array } from '../../../utils/push_array';
import Wrapper from './shared/Wrapper';
import Renderer from '../Renderer';
import Block from '../Block';
Expand Down Expand Up @@ -166,7 +167,7 @@ export default class IfBlockWrapper extends Wrapper {
block.has_outro_method = has_outros;
});

renderer.blocks.push(...blocks);
push_array(renderer.blocks, blocks);
}

render(
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/preprocess/decode_sourcemap.ts
@@ -1,3 +1,4 @@
import { push_array } from '../utils/push_array';
import { decode as decode_mappings } from 'sourcemap-codec';
import { Processed } from './types';

Expand Down Expand Up @@ -50,7 +51,7 @@ function decoded_sourcemap_from_generator(generator: any) {
result_segment = result_line[result_line.length - 1];

if (mapping.source != null) {
result_segment.push(...[
push_array(result_segment, [
source_idx[mapping.source],
mapping.originalLine - 1, // line is one-based
mapping.originalColumn
Expand Down
5 changes: 3 additions & 2 deletions src/compiler/preprocess/index.ts
@@ -1,3 +1,4 @@
import { push_array } from '../utils/push_array';
import { RawSourceMap, DecodedSourceMap } from '@ampproject/remapping/dist/types/types';
import { getLocator } from 'locate-character';
import { MappedCode, SourceLocation, parse_attached_sourcemap, sourcemap_add_offset, combine_sourcemaps } from '../utils/mapped_code';
Expand Down Expand Up @@ -48,7 +49,7 @@ class PreprocessResult implements Source {
}

if (dependencies) {
this.dependencies.push(...dependencies);
push_array(this.dependencies, dependencies);
}
}

Expand Down Expand Up @@ -165,7 +166,7 @@ async function process_tag(
});

if (!processed) return no_change();
if (processed.dependencies) dependencies.push(...processed.dependencies);
if (processed.dependencies) push_array(dependencies, processed.dependencies);
if (!processed.map && processed.code === content) return no_change();

return processed_tag_to_code(processed, tag_name, attributes, slice_source(content, tag_offset, source));
Expand Down
11 changes: 11 additions & 0 deletions src/compiler/utils/push_array.ts
@@ -0,0 +1,11 @@
export function push_array(thisArray: any[], ...otherList: any[]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, do we even need this method? It looks like we could just call concat everywhere this is being called for the same effect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*usually* push is faster than concat. depends on the exact input data. i can add the concat version for benchmarks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are talking about the compiler step here (not runtime), do we really need to be that performance sensitive here if it doesn't make a difference of more than 0.1 ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

benchmarks to the rescue ... in the worst case, this is hot code

let count = 0;
for (let a = 0; a < otherList.length; a++) {
const other = otherList[a];
for (let i = 0; i < other.length; i++) {
thisArray.push(other[i]);
}
count += other.length;
}
return count;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to return count? the return value doesn't seem to ever be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compatibility with Array.prototype.push, almost zero cost

}