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

Perf regression in latest release (1.15.0) #2246

Closed
stevenle opened this issue Feb 16, 2024 · 9 comments · Fixed by #2247
Closed

Perf regression in latest release (1.15.0) #2246

stevenle opened this issue Feb 16, 2024 · 9 comments · Fixed by #2247
Milestone

Comments

@stevenle
Copy link

stevenle commented Feb 16, 2024

Description

Overnight, the latest release (1.15.0) seemed to have caused a performance regression. Our "htmlPretty" function went from finishing in a few ms to taking 20s to complete. We've pinned our dependency to the last known good version that worked for us (1.14.9) to resolve the issue.

Input

export async function htmlPretty(
  html: string,
  options?: HtmlPrettyOptions
): Promise<string> {
  const prettyOptions = options || {
    indent_size: 0,
    end_with_newline: true,
    extra_liners: [],
  };
  try {
    const output = beautify.html(html, prettyOptions);
    return output.trimStart();
  } catch (e) {
    console.error('failed to pretty html:', e);
    return html;
  }
}

Expected Output

Expected to finish within X ms

Actual Output

The function is taking about 20s to complete, and the returned HTML is not pretty-printed.

@bitwiseman
Copy link
Member

Fixed in 1.15.1. Thanks for the fast report.

@bitwiseman
Copy link
Member

bitwiseman commented Feb 16, 2024

@stevenle How big are your html files? Would you be willing to share one that was showed the problem (for future testing)?

@stevenle
Copy link
Author

@bitwiseman Appreciate the quick response. Re: testing, I'll need to isolate a test for you, but afaik it was affecting us on a few different websites both small and large. I'm traveling today but I can try to set up a quick stackblitz example for you next week.

@bitwiseman
Copy link
Member

@stevenle No hurry, thanks again.

@Tortila90
Copy link

@stevenle Could you please share one of the problematic HTML files?

@stevenle
Copy link
Author

stevenle commented Apr 4, 2024

@Tortila90 I actually tried to create a stackblitz isolating the problem but wasn't able to re-create. Maybe there was a transitive dependency that fixed the problem, or maybe it was something else. After pinning js-beautify to an exact version we haven't had issues so I think we're good here on my end. If I encounter issues again I'll try to isolate the problem for you guys.

@Tortila90
Copy link

@bitwiseman Are you aware of any other performance issues when angular is turned on by default? Maybe we should consider reverting these changes?

@simon-knu
Copy link

Do you consider to add the angular option by default at one point since it's seems there is not any issues ? Or is it too touchy ?

@bitwiseman
Copy link
Member

@simon-knu
I've generally followed the policy of make new features off by default. Almost every time I've made an exception to that policy it has broken or caused issues for some significant set of users. "Not any issue" in this case is "not any known issue". I don't have the time to deal with fall out from a new way this feature would cause breaks.

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 a pull request may close this issue.

4 participants