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

Printing arguments: short circuiting some cases #6229

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

duailibe
Copy link
Member

So when investigating #4784, I've managed to verify the issue was in printArgumentsList(), more specifically the part where we check if it should hug the first of the last argument, and later, verified the issue was in the conditionalGroup, and more specifically, the first possible group, with a combination of ifBreak() and softline.

The previous code had the following meaning: if the group breaks, print all arguments broken down; else, just concat the arguments with the wrapping parenthesis (note that there are some cases where we print a new line but the group doesn't break!).

So the first thing I changed was: if none of the arguments break beforehand (this is before printing, so this means the argument "doc" already includes something that makes it break. It can break later when printing if it exceeds the print width), we can try a simple concat. If it fails, it will continue to the next cases in the conditional group. And only if one of the arguments break beforehand, we use the ifBreak().. another change is instead of using softline for the ifBreak case, just print a hardline instead.

These changes didn't break any existing tests and fix the performance degradation.

var prettier = require("./");
var code = `
var render = function () {var _vm=this;var _h=_vm.$createElement;var _c=_vm._self._c||_h;return _c('layout-manager',[_c('p',[_vm._v("Hello!")]),_vm._v(" "),_c('p',{staticClass:"katex-block"},[_c('span',{staticClass:"katex-display"},[_c('span',{staticClass:"katex"},[_c('span',{staticClass:"katex-mathml"},[_c('math',[_c('semantics',[_c('mrow',[_c('mfrac',[_c('mrow',[_c('msub',[_c('mi',[_vm._v("A")]),_c('mn',[_vm._v("1")])],1),_c('msub',[_c('mi',[_vm._v("B")]),_c('mn',[_vm._v("2")])],1)],1),_c('mi',[_vm._v("C")])],1)],1),_c('annotation',{attrs:{"encoding":"application/x-tex"}},[_vm._v("\\frac{A_1B_2}{C}")])],1)],1)],1),_c('span',{staticClass:"katex-html",attrs:{"aria-hidden":"true"}},[_c('span',{staticClass:"base"},[_c('span',{staticClass:"strut",staticStyle:{"height":"2.04633em","vertical-align":"-0.686em"}}),_c('span',{staticClass:"mord"},[_c('span',{staticClass:"mopen nulldelimiter"}),_c('span',{staticClass:"mfrac"},[_c('span',{staticClass:"vlist-t vlist-t2"},[_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"1.36033em"}},[_c('span',{staticStyle:{"top":"-2.314em"}},[_c('span',{staticClass:"pstrut",staticStyle:{"height":"3em"}}),_c('span',{staticClass:"mord"},[_c('span',{staticClass:"mord mathdefault",staticStyle:{"margin-right":"0.07153em"}},[_vm._v("C")])])]),_c('span',{staticStyle:{"top":"-3.23em"}},[_c('span',{staticClass:"pstrut",staticStyle:{"height":"3em"}}),_c('span',{staticClass:"frac-line",staticStyle:{"border-bottom-width":"0.04em"}})]),_c('span',{staticStyle:{"top":"-3.677em"}},[_c('span',{staticClass:"pstrut",staticStyle:{"height":"3em"}}),_c('span',{staticClass:"mord"},[_c('span',{staticClass:"mord"},[_c('span',{staticClass:"mord mathdefault"},[_vm._v("A")]),_c('span',{staticClass:"msupsub"},[_c('span',{staticClass:"vlist-t vlist-t2"},[_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"0.30110799999999993em"}},[_c('span',{staticStyle:{"top":"-2.5500000000000003em","margin-left":"0em","margin-right":"0.05em"}},[_c('span',{staticClass:"pstrut",staticStyle:{"height":"2.7em"}}),_c('span',{staticClass:"sizing reset-size6 size3 mtight"},[_c('span',{staticClass:"mord mtight"},[_vm._v("1")])])])]),_c('span',{staticClass:"vlist-s"},[_vm._v("​")])]),_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"0.15em"}},[_c('span')])])])])]),_c('span',{staticClass:"mord"},[_c('span',{staticClass:"mord mathdefault",staticStyle:{"margin-right":"0.05017em"}},[_vm._v("B")]),_c('span',{staticClass:"msupsub"},[_c('span',{staticClass:"vlist-t vlist-t2"},[_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"0.30110799999999993em"}},[_c('span',{staticStyle:{"top":"-2.5500000000000003em","margin-left":"-0.05017em","margin-right":"0.05em"}},[_c('span',{staticClass:"pstrut",staticStyle:{"height":"2.7em"}}),_c('span',{staticClass:"sizing reset-size6 size3 mtight"},[_c('span',{staticClass:"mord mtight"},[_vm._v("2")])])])]),_c('span',{staticClass:"vlist-s"},[_vm._v("​")])]),_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"0.15em"}},[_c('span')])])])])])])])]),_c('span',{staticClass:"vlist-s"},[_vm._v("​")])]),_c('span',{staticClass:"vlist-r"},[_c('span',{staticClass:"vlist",staticStyle:{"height":"0.686em"}},[_c('span')])])])]),_c('span',{staticClass:"mclose nulldelimiter"})])])])])])])])}
var staticRenderFns = []
render._withStripped = true
`;

prettier.format(code, { parser: "babel", trailingComma: "all" });

The execution time comes down to 0.58s.

> time node test.js
node test.js  0.58s user 0.14s system 89% cpu 0.799 total

Closes #4784

@duailibe
Copy link
Member Author

Even though this didn't break any tests, this is one of the most sensitive parts of our code.

cc @lydell

@lydell
Copy link
Member

lydell commented Jun 17, 2019

I'm sorry, I don't understand this piece of code well enough to say whether this is safe. The diff does looks reasonable though, and I think it's worth taking a chance to fix this nasty performance issue.

@duailibe duailibe merged commit 8fcc7c1 into prettier:master Jun 17, 2019
@duailibe duailibe deleted the arguments branch June 17, 2019 09:14
@j-f1
Copy link
Member

j-f1 commented Jun 24, 2019

Should we have some sort of perf test suite that includes inputs like this one?

@duailibe
Copy link
Member Author

My first reaction is I don’t think so, but I’m not sure

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 23, 2019
@thorn0 thorn0 added the type:perf Issue with performance of Prettier label May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:perf Issue with performance of Prettier
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Severe performance degradation on small code. 5+ minutes to format
4 participants