Skip to content

createNodeArray mutates its input if not already a NodeArray #20471

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

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 5, 2017

Fixes #20429 (comment)
The input to createNodeArray should not be a ReadonlyArray since it's about to be cast to NodeArray and then mutated. Combined with emptyArray this was mutating global state.

@ghost ghost requested a review from rbuckton December 5, 2017 16:31
@ghost ghost mentioned this pull request Dec 5, 2017
@ghost ghost force-pushed the createNodeArray branch from 20f1b2c to d9d9c7f Compare December 5, 2017 16:37
@ghost ghost force-pushed the createNodeArray branch from d9d9c7f to dff9578 Compare December 5, 2017 17:16
}

const array = <NodeArray<T>>elements;
const array = elements as MutableNodeArray<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, if its not already a NodeArray, we should instead clone it via .slice() before modifying it? Then we wouldn't need the Nodes<T> type, though at first glance I'm not certain what the memory/performance impact would be.

Copy link
Author

Choose a reason for hiding this comment

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

I tested out adding slice inside of createNodeArray (at the bottom of the if (elements) block), and actually saw a slight decrease in emit time for some reason -- might have to do with the fact that createNodeArray would otherwise have to change the shape of an array object acquired from elsewhere, but if we create a fresh slice it may be allocated with the correct shape already.

The most efficient option may be to avoid ever calling createNodeArray on an existing array, and instead always start by calling createNodeArray to get an empty mutable array, then pushing to it.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 12, 2018

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@ghost ghost deleted the createNodeArray branch November 9, 2018 18:30
@ghost ghost restored the createNodeArray branch November 9, 2018 19:50
@ghost ghost deleted the createNodeArray branch November 9, 2018 19:50
@ghost ghost restored the createNodeArray branch November 9, 2018 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants