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

🐛 Biome cpu spinning forverr on save, suspected sluggish organizeImports behaviour #98

Closed
1 task done
jpike88 opened this issue Aug 31, 2023 · 21 comments
Closed
1 task done
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@jpike88
Copy link

jpike88 commented Aug 31, 2023

Environment information

CLI:
  Version:                      1.0.0
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v16.14.2"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/8.5.0"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           true
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Discovering running Biome servers...

Incompatible Biome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Biome.

Server:
  Version:                      12.1.3

Incompatible Biome Server: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

ℹ Rage discovered this running server using an incompatible version of Biome.

Server:
  Version:                      <=10.0.0

Server:
  Status:                       stopped

What happened?

  1. pull down this repo and npm install: https://github.com/jpike88/biome-slow
  2. open the code workspace in root directory
  3. tweak something in eventDefinitions.ts, hit save.
  4. observe as it's stuck saving, and biome maxes out cpu indefinitely

this problem happened in Rome as well, it's same as rome/tools#4757

Expected result

it should just work

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@jpike88 jpike88 changed the title 🐛 Biome cpu spinning forverr on save 🐛 Biome cpu spinning forverr on save, suspected sluggish organizeImports behaviour Aug 31, 2023
@ematipico
Copy link
Member

ematipico commented Aug 31, 2023

Thank you for reporting. Can you disable the linter and let us know if you still have the issue?

@jpike88
Copy link
Author

jpike88 commented Aug 31, 2023

disabling linter saved instantly. having organize "source.organizeImports.biome": true was the culprit, it's definitely biome

@KaHLK
Copy link
Contributor

KaHLK commented Aug 31, 2023

If I disable everything (organizeImports & linter), it doesn't cause the cpu to spin up.

--- a/biome.json
+++ b/biome.json
@@ -3,6 +3,9 @@
        "formatter": {
                "enabled": false
        },
+       "organizeImports": {
+               "enabled": false
+       },
        "javascript": {
                "formatter": {
                        "quoteStyle": "single",
@@ -13,7 +16,7 @@
                }
        },
        "linter": {
-               "enabled": true,
+               "enabled": false,
                "rules": {
                        "complexity": {
                                "noExtraBooleanCast": "off",

Re-enabling the linter and formatter, does not cause it to spin up cpu either. But re-enabling the organizeImports causes the cpu to spin up again.

biome rage output

@ematipico
Copy link
Member

Amazing, thank you for the detailed report. If we're able to identify the code that causes the issue, we can fix the issue real quick

@ematipico ematipico added the S-Bug-confirmed Status: report has been confirmed as a valid bug label Aug 31, 2023
@Conaclos Conaclos added L-JavaScript Language: JavaScript and super languages A-Linter Area: linter labels Sep 1, 2023
@ematipico
Copy link
Member

ematipico commented Sep 5, 2023

I am trying the repository provided, and I am seeing different results. When I disable only the import sorting and run biome check --apply, the command takes up to 5 seconds. Running the check alone is fast enough.

This means that the cause of the issue is not the import sorting but the linter where some recommended rule applies a safe action that takes a long time.

I also tried to keep only the imports from the code, and the import sorting is snappy.

@Conaclos, this is a severe regression that we have to investigate ASAP.

@ematipico
Copy link
Member

The rule noUselessConstructor is the one that is taking a lot of time. I disabled it and now the check --apply command is snappy.

@jpike88
Copy link
Author

jpike88 commented Sep 5, 2023

weird, cause I had noUselessConstructor set to off in my biome.json

@Conaclos
Copy link
Member

Conaclos commented Sep 5, 2023

This is strange because the code fix of noUselessConstructor is really simple.

@Conaclos
Copy link
Member

Conaclos commented Sep 5, 2023

However, you are right, something is going wrong with the rule. It is maybe unrelated to this issue.

@ematipico
Copy link
Member

@jpike88 @KaHLK for the time being you can turn off the noUselessConstructor rule.

@jpike88
Copy link
Author

jpike88 commented Sep 5, 2023

Ok but I never had it turned on in my case, seems I already respect that rule anyway ;D

@ematipico
Copy link
Member

Ok but I never had it turned on in my case, seems I already respect that rule anyway ;D

It's part of the recommended rules: https://biomejs.dev/linter/rules/no-useless-constructor/

@KaHLK
Copy link
Contributor

KaHLK commented Sep 5, 2023

It's still a bit weird that I encountered the exact same behaviour back in [email protected] where noUselessConstructor was not yet enabled as a recommended rule (at least if I understand correctly).
At least based on the assumption that it is the same issue as experienced here.

It does however seem to be mostly mitigated by setting noUselessContstructor to off. Though I still sometime experience it slowing down a lot, with organizeImports on.

@ematipico
Copy link
Member

ematipico commented Sep 5, 2023

The origin of the issue for that particular code is given by that rule, but the underlying issue is not the rule itself but how we process the code fixes. This gets big when you have a big file with a certain number of code fixes. In this particular case, the file goes under many fixes triggered by noUselessContstructor

It's an issue we know of, but the fix requires some time to get right.

I want to keep the issue open because this kind of performance regression should not happen.

It does however seem to be mostly mitigated by setting noUselessContstructor to off. Though I still sometime experience it slowing down a lot, with organizeImports on.

Which make sense. organizeImports uses the same engine to create the new content.

@KaHLK
Copy link
Contributor

KaHLK commented Sep 5, 2023

The origin of the issue for that particular code is given by that rule, but the underlying issue is not the rule itself but how we process the code fixes. This gets big when you have a big file with a certain number of code fixes. In this particular case, the file goes under many fixes triggered by noUselessContstructor

It's an issue we know of, but the fix requires some time to get right.

I want to keep the issue open because this kind of performance regression should not happen.

It does however seem to be mostly mitigated by setting noUselessContstructor to off. Though I still sometime experience it slowing down a lot, with organizeImports on.

Which make sense. organizeImports uses the same engine to create the new content.

Makes sense 😄 Thanks for the fast response and great explanation 👍

@jpike88
Copy link
Author

jpike88 commented Sep 5, 2023

organizeImports uses the same engine to create the new content

Ok now I get it. I had noUselessConstructor explicitly turned off on purpose to replicate my eslint settings from the beginning, and perceived the issue from toggling organizeImports.

@yckbilly1929
Copy link

yckbilly1929 commented Sep 18, 2023

also facing similar issue
I'm working on a medium-size in-house project, after upgrading to version 1.2.2
biome check --apply-unsafe hangs forever

@ematipico
Copy link
Member

To whoever comes here, it's important to share a snippet that triggers the problem. Without that, we are basically useless and we can't help you.

@jpike88
Copy link
Author

jpike88 commented Nov 2, 2023

for what it's worth, still a problem on v1.3.3 with that initial repo I provided.... any closer to figuring it out?

@ematipico
Copy link
Member

ematipico commented Nov 2, 2023

for what it's worth, still a problem on v1.3.3 with that initial repo I provided.... any closer to figuring it out?

I did triage your issue a time ago, and I managed to reduce the problem to a single file. I ran biome check on that file yesterday and I didn't find any performance issues. Are you able to provide more information that we have logging now?

@jpike88
Copy link
Author

jpike88 commented Nov 2, 2023

I'm a dummy, 1.3.3 actually works fine! I forgot we moved back cause of #639. I'll close this one as solved for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

No branches or pull requests

5 participants