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

allow `import` inside `block`: makes N runnableExamples run N x faster, minimizes scope pollution #9300

Closed
timotheecour opened this Issue Oct 10, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@timotheecour
Copy link
Contributor

timotheecour commented Oct 10, 2018

proposal

allow import statements inside a block, as in following code:

block:
  import os

It currently gives: Error: 'import' is only allowed at top level.

rationale

  • currently, nim doc foo.nim where foo.nim contains N runnableExamples blocks will generate N files and N compilations
    I measured (on synthetic test cases; I can share code if needed) that running a single compilation+run on a file that concatenates N runnable tests runs N times faster than running the N runnable tests separately, since the overhead is almost always compilation step (+ overhead of starting a process etc). For N=50 (not unreasonable) that's a 50X speedup.

As we improve our docs with more runnableExamples, this problem is gonna become more and more pressing. I often run into timeout issues on travis already (#8707), and appveyor is still taking often > 8 hours to process a build (https://github.com/nim-lang/Nim/issues/8640)

  • allowing import inside a block can improve code health in a number of cases, avoiding symbol pollution and symbol clashes from too many top-level imports

  • EDIT likewise with tests: this could be used for PR's like #9318 which refactors several tests into 1 file (again, for improving speed of running tests)

Note

even if .. code-block:: :test: were used instead of runnableExamples, the exact same problem would be there (code is same underneath): the reason we're splitting runnableExamples into seperate files currently is to support import inside runnableExamples, which is needed for various reasons (eg allowing importing other modules inside a runnableExamples block without polluting the original module amongst other use cases, and making each runnableExamples independent scope-wise).

@timotheecour timotheecour changed the title allow `import` inside block: makes N runnableExamples run N x faster, minimizes scope pollution allow `import` inside `block`: makes N runnableExamples run N x faster, minimizes scope pollution Oct 10, 2018

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 11, 2018

Terrible. Took D specialized scoping rules to recover from this feature.

@krux02

This comment has been minimized.

Copy link
Contributor

krux02 commented Oct 13, 2018

From my experience with that feature in scala, I think it is not a good feature. When you have this feature, people will optimize "scope pollution" as you said it. But there isn't a real benefit to it, it's just a waste of time. Initially I did optimize this "scope pollution" but after I started using intellij which handles global imports automatically and I never had problems with it, I could dismiss local imports easily and never look back.

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

timotheecour commented Oct 13, 2018

my proposal addresses the speed problem of running nim doc ; what else do you recommend ?

eg: nim doc lib/pure/strutils.nim takes currently 10 seconds; with this it'd take ~ 1 second.
This will grow over time as runnableExamples are added. (and as I said above: .. code-block:: :test: would be just the same)

@krux02

This comment has been minimized.

Copy link
Contributor

krux02 commented Oct 13, 2018

well I guess it is easy to support an import statement in the runableExamples block.

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

timotheecour commented Oct 13, 2018

well I guess it is easy to support an import statement in the runableExamples block.

that's already supported, and that was (IIRC) the reason why runnableExamples had to be compiled in separate files to avoid symbol clashing and other issues [1] : each runnableExamples should be independent scope wise.
With why I proposed, we could go back to having a single file to run all runnableExamples in a given module (or even multiple ones!)

[1] eg of one type of problem that can happen if runnableExamples are run in 1 single file without this proposal:

proc foo1()
  runnableExamples:
   import bar
   foo1(barfoo)
  ...
proc foo2()
  runnableExamples:
   # oups, forgot to `import bar`
   foo1(barfoo)
nim doc main.nim` # works by "accident" because `import bar` from foo1's runnableExamples spills over to foo2's runnableExamples

but if a user copy pastes runnableExamples from foo2 into an editor, it will fail because import bar from runnableExamples would be missing: this makes runnableExamples less useful since they're not standalone.

With this proposal, this would all work (and have the Nx speed benefit mentioned)

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 15, 2018

We can also only split the runnable examples into multiple files if different import statements are used.

@krux02

This comment has been minimized.

Copy link
Contributor

krux02 commented Oct 15, 2018

Sounds like a good idea. But if it's done, It should be mentioned in the documentation of runnableExamples so that users have a chance to optimize the runnable examples to have identical imports.

@krux02 krux02 added Performance and removed speed labels Nov 1, 2018

@Araq Araq closed this in 0862294 Jan 4, 2019

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

timotheecour commented Jan 4, 2019

here's some benchmark:

before 0862294

gtime -v nim doc lib/pure/strutils.nim
Command being timed: "nim doc lib/pure/strutils.nim"
User time (seconds): 29.92
System time (seconds): 5.15
Percent of CPU this job got: 105%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:33.14

after:

Command being timed: "./bin/nim doc lib/pure/strutils.nim"
User time (seconds): 3.19
System time (seconds): 1.61
Percent of CPU this job got: 228%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:02.10

so that's a huge time saving, and one that'll only grow over time if runnableExamples becomes more common (which I hope).

Note: the previous issues we had with running all runnableExamples in a single file (eg causing some conflicts eg symbol clashes with multiple imports) are avoided in that commit by 1 level of indirection, each runnableExample still only has its own imports and nothing more, eg:

import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples1.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples2.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples3.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples4.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples5.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples6.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples7.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples8.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples9.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples10.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples11.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples12.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples13.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples14.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples15.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples16.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples17.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples18.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples19.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples20.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples21.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples22.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples23.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples24.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples25.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples26.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples27.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples28.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples29.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples30.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples31.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples32.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples33.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples34.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples35.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples36.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples37.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples38.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples39.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples40.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples41.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples42.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples43.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples44.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples45.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples46.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples47.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples48.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples49.nim"
import r"/Users/timothee/.cache/nim/strutils_d/runnableExamples/strutils_examples50.nim"

so, thanks @Araq for the fix!

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