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

Mutate the doc.parts array when printing fill #3273

Merged
merged 2 commits into from Nov 16, 2017

Conversation

Projects
None yet
4 participants
@duailibe
Member

duailibe commented Nov 15, 2017

Context: #3263 (comment)

Slicing the array means copying the array's content for every pair, which makes the algorithm quadratic. This PR changes the copying to mutating the array directly to prevent that. It's a more dangerous approach but the way fill works it's a valid solution.

I'm not 100% comfortable with that and I'll take suggestions.

const secondContent = parts[2];
parts.splice(0, 2);

This comment has been minimized.

@suchipi

suchipi Nov 15, 2017

Member

A comment noting why we're mutating here might be useful.

@josephfrazier

This comment has been minimized.

Collaborator

josephfrazier commented Nov 15, 2017

This is a bit out of scope, but what if we used something like Immutable.js to allow us to avoid mutation without impacting performance (or with very little impact)? Obviously this would be a larger, separate change, but I wanted to throw it out there.

@suchipi

This comment has been minimized.

Member

suchipi commented Nov 15, 2017

I think using immutable structures is a good idea to keep in mind... but adding it at this point seems excessive. Maybe if general performance concerns start to be more pressing, we should look into it.

@duailibe duailibe merged commit 8e5c335 into prettier:master Nov 16, 2017

3 checks passed

codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 97.07% (+0%) compared to 2332c5c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@duailibe duailibe deleted the duailibe:fill-slow-slice branch Nov 16, 2017

@vjeux

This comment has been minimized.

Collaborator

vjeux commented Nov 25, 2017

@josephfrazier immutable.js has a non trivial performance impact. In a React context, the performance overhead of the immutable data structures on-top of the js ones is offset by the wins you get from shouldComponentUpdate.

In our case, those data structures are pretty performance sensitive, so I'd be willing to be that we'll see a non trivial regression.

@suchipi suchipi added this to the 1.9 milestone Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment