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

Bail when traversing === groups #1294

Merged
merged 1 commit into from Apr 18, 2017

Conversation

Projects
None yet
3 participants
@vjeux
Copy link
Collaborator

vjeux commented Apr 16, 2017

This is the second part of the fix for the performance regression seen in #1250. In #1217, for correctness reasons, we're now traversing all the conditional groups. This means that we're now in O(n^2). But, in practice, many of those groups are === between each others. So we only need to recurse through one of the instances to know if it's going to break.

This makes the first example go from not terminating to being instant. The second one going from not terminating to taking ~1s. We can also make it instant by tweaking the printing phase, but that's for another PR.

@@ -1,34 +1,35 @@
"use strict";

function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) {
var hasStopped = false;

This comment has been minimized.

Copy link
@vjeux

vjeux Apr 16, 2017

Author Collaborator

hasStopped is a too aggressive for our new use case, we want to avoid going down recursively, but not stop the traversal altogether. Fortunately, we can change the behavior where false means that we should stop recursing and implement the hasStopped behavior in the call site that needs it.

result = maybeResult;
}
if (hasStopped) {

This comment has been minimized.

Copy link
@vjeux

vjeux Apr 16, 2017

Author Collaborator

here

doc.expandedStates.forEach(traverseDocRec);
} else {
if (shouldRecurse) {
if (doc.type === "concat") {

This comment has been minimized.

Copy link
@vjeux

vjeux Apr 16, 2017

Author Collaborator

All this is the same, just indented one more, sadly github makes it way scarier.

This comment has been minimized.

Copy link
@SimenB

SimenB Apr 16, 2017

Contributor

https://github.com/prettier/prettier/pull/1294/files?w=1 makes github ignore whitespace in diffs. Sadly you can't comment, but at least it makes it readable 😄

@@ -120,6 +125,7 @@ function breakParentGroup(groupStack) {
}

function propagateBreaks(doc) {
var alreadyVisited = new WeakMap();

This comment has been minimized.

Copy link
@jlongster

jlongster Apr 17, 2017

Member

Theoretically this could just be a Map, right? This goes out of scope anyway and will be GCed so the strong reference will go away anyway.

This comment has been minimized.

Copy link
@vjeux

vjeux Apr 18, 2017

Author Collaborator

Yeah, at first I thought the problem was going to be widespread around the entire codebase so I needed to make it global and not hold any strong reference. But it turned out to only be scoped to this function so a Map is enough indeed.

@jlongster
Copy link
Member

jlongster left a comment

One small comment, but this looks really good! Thanks for improving it!

@jlongster
Copy link
Member

jlongster left a comment

Oops, I didn't mean to force any changes.

Bail when traversing === groups
This is the second part of the fix for the performance regression seen in #1250. In #1217, for correctness reasons, we're now traversing all the conditional groups. This means that we're now in O(n^2). But, in practice, many of those groups are === between each others. So we only need to recurse through one of the instances to know if it's going to break.

This makes the first example go from not terminating to being instant. The second one going from not terminating to taking ~1s. We can also make it instant by tweaking the printing phase, but that's for another PR.

@vjeux vjeux force-pushed the vjeux:weak_map branch from 00c75a7 to a67da09 Apr 18, 2017

@vjeux vjeux merged commit 5995af2 into prettier:master Apr 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.