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

Finding engine is pretty slow #807

Closed
elibarzilay opened this issue Jul 20, 2024 · 8 comments · Fixed by #810
Closed

Finding engine is pretty slow #807

elibarzilay opened this issue Jul 20, 2024 · 8 comments · Fixed by #810

Comments

@elibarzilay
Copy link
Contributor

Since it uses node to find the version, and starting it is kind of slow.

If it's ok to use jq, I'll be happy to make a PR that uses it if it exists, or use node otherwise.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jul 20, 2024

From some experimentation, the slowest part is using semver to resolve a complex version expression. Are you seeing a progress line containing "resolving"?

Relatively slow with complex expression:

% time n lsr engine
       found : /Users/john/Documents/Sandpits/n/issues/807/package.json
        read : >=14.6 <19.0.0
   resolving : >=14.6 <19.0.0
18.20.4
n lsr engine  1.47s user 0.38s system 86% cpu 2.149 total

Relatively fast with simple expression that is recognised by n and processed directly:

% time n lsr engine
       found : /Users/john/Documents/Sandpits/n/issues/807/package.json
        read : >18.0.0
      target : current
22.5.1
n lsr engine  0.06s user 0.04s system 75% cpu 0.141 total

The block of code that contains that progress uses npx to execute semver to resolve the expression:

n/bin/n

Line 1112 in 76d6b98

verbose_log "resolving" "${range}"

@elibarzilay
Copy link
Contributor Author

elibarzilay commented Jul 20, 2024

I'm talking about the simple case, where just starting node is much slower than starting jq. When a resolution is needed, there's obviously no good way to avoid going through the code.

For example, n which auto in the source directory takes an average 354ms, and with jq I get 169ms.

@shadowspawn
Copy link
Collaborator

For interest, what are you doing that you notice the slowness?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jul 21, 2024

If it's ok to use jq, I'll be happy to make a PR that uses it if it exists, or use node otherwise.

On the one hand, I don't particularly like checking for an optional tool. On the other hand, the engine support checks for node in the same way! I do like that would not need node installed for processing simple expressions if jq is available.

PR welcome, see how it goes. (And I would like jq version included in the doctor output.)

@elibarzilay
Copy link
Contributor Author

  1. Slowness: just running node has a significant startup overhead, with using node like this, that overhead is (at least) duplicated, which is how I noticed it. (So, just through plain interactive use.)
  2. An advantage of jq -- besides being smaller + faster -- is that it is a popular choice for CI pipelines, and since it's simple I figured it's worth the minor improvement.
  3. In addition, node can be popular in some setups, but n is useful in installing it in the first place for setups that lack a builtin node. So my guess is that there's good chances that there's a fair number of cases that would be simplified.

@elibarzilay
Copy link
Contributor Author

@shadowspawn Made the above PR.

Apologies, but I can't access my work machine at the moment, so I used the tweak that I did previously, with a bit of reshuffling to make it a proper PR. Hopefully I didn't make have any stupid typos.

@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Aug 16, 2024
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Sep 6, 2024
@shadowspawn
Copy link
Collaborator

Released in v10.0.0

@elibarzilay
Copy link
Contributor Author

Thanks!

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.

2 participants