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

nim dump is roughly 100x slower in 1.3 versus 1.2 #14179

Closed
disruptek opened this issue Apr 30, 2020 · 5 comments · Fixed by #14658
Closed

nim dump is roughly 100x slower in 1.3 versus 1.2 #14179

disruptek opened this issue Apr 30, 2020 · 5 comments · Fixed by #14658

Comments

@disruptek
Copy link
Contributor

See title.

@Araq
Copy link
Member

Araq commented May 1, 2020

I'm pretty sure it's caused by our NimScript config evaluation.

@disruptek
Copy link
Contributor Author

Confirmed. This is also the source of the extra output problem detailed in #13912.

@timotheecour

@timotheecour
Copy link
Member

timotheecour commented May 7, 2020

@Araq as always, the 1st thing to try is git bisect

3d2459bdc0b6d6236a2cd9209ed81c965ee411a5 is the first bad commit
commit 3d2459bdc0b6d6236a2cd9209ed81c965ee411a5
Author: Araq <rumpf_a@web.de>
Date:   Thu Apr 30 20:23:42 2020 +0200

    fixes the regression #12860 caused; hotfix

 lib/pure/os.nim          | 2 +-
 lib/system/nimscript.nim | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)
bisect run success

and indeed git bisect is correct once again:

3d2459b adds a import os dependency on nimscript which introduces the regression:

  • nim dump . processes $nim/config/config.nims
  • $nim/config/config.nims imports system => nimscript.nim => (since that commit) os.nim
  • os.nim itself is expensive, causing a 6X slowdown

But that's not the end of the story.

Because of dom96/choosenim#193 (which I reported here #13986 (comment) as a confounding factor for another issue), $nim/config/config.nims is not copied over to $nim_toolchain_D/nim-1.2.0/config/config.nims, so for nim 1.2.0 installed via choosenim you instead get:

  • nim dump . processes ... no nims file at all since processes $nim/config/config.nims doesn't exist

=> 23X difference

timings:

$nim_120_X dump --skipusercfg --hint:conf:on .  0.01s user 0.01s system 88% cpu 0.015 total
$nimb dump --skipusercfg --hint:conf:on .  0.05s user 0.01s system 97% cpu 0.062 total # right before 3d2459bdc0b6d6236a2cd9209ed81c965ee411a5
$nimb dump --skipusercfg --hint:conf:on .  0.33s user 0.02s system 99% cpu 0.352 total # right after 3d2459bdc0b6d6236a2cd9209ed81c965ee411a5

@disruptek

Confirmed. This is also the source of the extra output problem detailed in #13912.

unrelated AFAIU

proposal

That will fix all above problems including #14142.

local imports

timotheecour#169
That's a topic for another discussion, but allowing local imports (eg at proc scope) would solve many dependency issues, including the one that causes this regression, making importing more lazy. I even have a working branch for that.

@genotrance
Copy link
Contributor

dom96/choosenim#193 is a bug that keeps creeping up and should be fixed, $nim/config/config.nims is meant to be read for correctness even if it currently doesn't contain much; users who know what they're doing can always pass --skipCfg to skip it

This is not a choosenim issue which simply downloads the .zip or .xz file from nim-lang.org and extracts it. The file is missing in the archive posted on nim-lang.org.

@timotheecour
Copy link
Member

timotheecour commented May 7, 2020

The file is missing in the archive posted on nim-lang.org.

fixing that part in #14267

Araq pushed a commit that referenced this issue Jun 16, 2020
* fix #14142: no more clash with: import os + use of existsDir/dirExists/existsFile/fileExists/findExe in config.nims

* remove a comment

* Revert "fixes the regression #12860 caused; hotfix"

This reverts commit 3d2459b.

* Revert "Undefine `paramCount` & `paramStr` in nimscript.nim for *.nims (#12860)"

This reverts commit d38853c.

* noNimScript => noWeirdTarget + noNimJs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants