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

Even larger import/no-cycle performance downgrade in 2.30.0 #3047

Open
jzaefferer opened this issue Sep 4, 2024 · 16 comments
Open

Even larger import/no-cycle performance downgrade in 2.30.0 #3047

jzaefferer opened this issue Sep 4, 2024 · 16 comments

Comments

@jzaefferer
Copy link

#2348 was supposed to get fixed in 2.30.0, but for my project the performance of import/no-cycle got even worse.

With 2.29.1 it took 21s:

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle                         | 21503.212 |    52.9%

With 2.30.0 it took 67s, 3.2 times slower:

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle                         | 67440.935 |    76.8%

I also tried 'import/no-cycle': [2, { ignoreExternal: false, maxDepth: 3 }],, which was suggested in the previous ticket, but that made no performance difference. It did find more errors though.

Are there other measurements I can share? Like a cpu profile? I'm not sure what's useful here and didn't find good clues in #2348.

@diegodorado
Copy link

Same for me: it is worse now. Went from

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      |  5091.198 |    31.2%

to

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      |  6829.761 |    39.1%

@ljharb
Copy link
Member

ljharb commented Sep 4, 2024

What happens if you set the disableScc option to true? Does it restore the v2.29 speed?

cc @soryy708

@jzaefferer
Copy link
Author

@ljharb yep, it does

@soryy708
Copy link
Contributor

soryy708 commented Sep 5, 2024

Cool, glad we decided to put disableScc in.

The SCC is a pre-processing we do to speed up no-cycle to enable early-exit (short-circuiting). In large projects with many files to lint, it prunes a lot of unnecessary work, but in small projects the preprocessing can take longer than the time it saves.

@ljharb
Copy link
Member

ljharb commented Sep 5, 2024

Is there any way we could figure out a heuristic, and dynamically figure out whether it’ll be faster to use scc or not?

@soryy708
Copy link
Contributor

soryy708 commented Sep 5, 2024

Is there any way we could figure out a heuristic, and dynamically figure out whether it’ll be faster to use scc or not?

I don't have anything in mind. It depends on a lot of things - the user's hardware, cpu, how fast their hdd/ssd is...

If the heuristic will be right then great, the user won't need to configure anything.

But what if the heuristic will be wrong? They'll need to override the heuristic's decision, either to enable or to disable SCC.

@ljharb
Copy link
Member

ljharb commented Sep 5, 2024

That’s already the case - we just chose a heuristic of “always use scc”. The hope would be do minimize how many users have to enable or disable scc explicitly.

@soryy708
Copy link
Contributor

soryy708 commented Sep 5, 2024

IMO it's a good heuristic because:

  • If your project is small, lint takes seconds, so you don't care about performance as much, don't mind the overhead
  • If your project is large, lint takes minutes, so you care about performance, and want more efficiency

Defaults that cater to large codebases (perhaps at the expense of tiny ones) are good, while there is an escape hatch in case the user cares enough.

@ljharb
Copy link
Member

ljharb commented Sep 5, 2024

I totally agree that it's the right heuristic :-) i'm just wondering if we can fine-tune it a bit for projects in the 20-70s range.

@soryy708
Copy link
Contributor

What happens if you set the disableScc option to true? Does it restore the v2.29 speed?

cc @soryy708

Created an issue to add disableScc to the docs:

@soryy708
Copy link
Contributor

#3068

@soryy708
Copy link
Contributor

soryy708 commented Oct 7, 2024

@jzaefferer @diegodorado Please try version 2.31.0, it includes the fix from #3068.
Is performance better with/without disableScc?

@jzaefferer
Copy link
Author

With 2.31.0, the performance is much better with the default scc on (no disableScc used)

disableScc true: 

1 import/no-cycle                         | 28912.195 |    72.0%
2 import/no-cycle                         | 29854.613 |    71.6%
3 import/no-cycle                         | 28827.838 |    71.6%

default scc

1 import/no-cycle                         | 12021.782 |    51.5%
2 import/no-cycle                         | 11614.811 |    50.7%
3 import/no-cycle                         | 12250.667 |    50.3%

Very nice! Since I created this ticket, I'll take the liberty to also close it 💃

@TkDodo
Copy link

TkDodo commented Oct 9, 2024

Sadly, it didn’t fix the issue for us. After upgrading, the total lint time went from 1m36s to 2m54s on my machine (linting over multiple packages in a monorepo).

Here are the specific timings for one example package with our settings:

'import/no-cycle': [
    'error',
    {
        maxDepth: 3,
        allowUnsafeDynamicCyclicDependency: true,
    },
],

v2.31.0, disableScc: false (default)

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle                         | 27556.928 |    70.6%

and with disableScc: true:

import/no-cycle                         | 11824.795 |    51.2%

on v2.30.0, I’m getting the following results:

v2.30.0, disableScc: false (default)

import/no-cycle                         |  4719.848 |    30.0%

and with disableScc: true:

import/no-cycle                         |  2156.667 |    15.5%

So, all settings in v31 perform worse than v30.

@jzaefferer jzaefferer reopened this Oct 9, 2024
@ljharb
Copy link
Member

ljharb commented Oct 10, 2024

What happens if you remove maxDepth entirely?

@TkDodo
Copy link

TkDodo commented Oct 10, 2024

What happens if you remove maxDepth entirely?

for v30, it becomes a bit faster (with disableScc: true)

import/no-cycle                         |  2144.215 |    15.5%

for v31, still not really better

disableScc: false

import/no-cycle                         | 26919.054 |    70.4%

disableScc: true:

import/no-cycle                         | 27324.862 |    71.0%

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

No branches or pull requests

5 participants