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

Load crystal stdlib in background during REPL startup #7

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

Vici37
Copy link
Contributor

@Vici37 Vici37 commented Jun 20, 2022

Gist is that the stdlib could be more efficiently loaded in the background while the user is entering in their first line :) This cuts REPL startup time from ~1/2 second to instantaneously, and everything loads correctly.

I tested adding a sleep 30 in the spawn below to simulate user input before everything was loaded, and ended up seeing a much shorter list in the tab complete of what I could do (as expected). Waiting out the 30 seconds had everything load in the same session and tab complete "fixed" itself.

@I3oris
Copy link
Owner

I3oris commented Jun 20, 2022

Hello Vici37, thank you for coming with this.

The prompt starts instantaneously but then, the user get stuck (with no echo of its input) during the loading of the prelude. Adding the sleep 30 make the editing loop work correctly during these 30 second, but the prelude is only loaded after the delay and block the user until it finish. This end up to be just equivalent to display the prompt before loading prelude :/.

You can better see what happen (or when) with this:

spawn do
  puts "load prelude"
  load_prelude
  puts "prelude loaded!"
end 

I have already thought to this problem before, but didn't found a easy solution. The main problem is that load_prelude will always be executed at once, with no way to execute code between (output echo, tabulation...). I think it's due to concurrency model.

Anyway, maybe just display the prompt before loading the prelude would be fine for now, it still better than wait before the prompt^^.

EDIT: This works fine with parallelism enabled! (flag -Dpreview_mt) Prelude is loaded in background while you can still edit the expression 😀.

The only thing to do then is to ensure than prelude is loaded before interpreting the code.

@I3oris I3oris added the enhancement New feature or request label Jun 20, 2022
@Vici37
Copy link
Contributor Author

Vici37 commented Jun 21, 2022

Ah, good explanation with the REPL behavior - I noticed it after I set up the PR, but still found it a better user experience than a delayed initial prompt displaying :)

Great find with the parallel loading! I'll experiment with thi, channels could probably be uses here to halt interpretation until loading is complete from the other thread. I'll try and update this PR tomorrow if I have time.

@@ -18,4 +18,3 @@ scripts:
development_dependencies:
ameba:
github: crystal-ameba/ameba
version: ~> 0.14.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ameba is now 1.0.0, so removing this version constraint

@@ -12,7 +12,6 @@ module IC::ReplInterface
@auto_completion = AutoCompletionHandler.new
@history = History.new
@prelude_complete_channel : Channel(Int32)? = nil
@prelude_complete = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out to not be needed, can check if the channel 1) exists and 2) hasn't been closed yet instead.

Comment on lines 137 to 143
private def check_prelude_complete
return if @prelude_complete
return unless channel = @prelude_complete_channel
return if channel.closed?

channel.receive
@prelude_complete = true
channel.close
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meat of the change is this new method

@Vici37
Copy link
Contributor Author

Vici37 commented Jun 21, 2022

For testing, I found p to be the shortest valid command I could enter.

@I3oris
Copy link
Owner

I3oris commented Jun 21, 2022

Woaw great, thank you for the PR!

@I3oris
Copy link
Owner

I3oris commented Jun 21, 2022

Loving see the interpreter load instant' ! 🎉

@Vici37
Copy link
Contributor Author

Vici37 commented Jun 23, 2022

Merge at your leisure, unless you have review comments I missed, I have nothing more to add :D

@I3oris
Copy link
Owner

I3oris commented Jun 24, 2022

Oh!, actually I have, I didn't realize that my review was hidden! I didn't know that pending review wasn't public, my bad.

Makefile Outdated
@@ -3,7 +3,7 @@ CRYSTAL_CONFIG_PATH ?= '$$ORIGIN/../share/crystal-ic/src'
CRYSTAL_LIB_CONFIG_PATH ?= '$$ORIGIN/../lib/ic/share/crystal-ic/src'

COMPILER ?= crystal
FLAGS ?= --progress
FLAGS ?= -Dpreview_mt --progress
RELEASE_FLAGS ?= --progress --release
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RELEASE_FLAGS ?= --progress --release
RELEASE_FLAGS ?= -Dpreview_mt --progress --release

Could be good also in release mode :)

private def on_enter(&)
check_prelude_complete
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the check could also goes at line 183. Just before yielding for interpreting code.

submit_expr do
  check_prelude_complete
  yield @editor.expression
end

Even better actually, I think it could goes inside the yielded block, so directly in repl.cr:

repl_interface.run do |expr|
  check_prelude_complete(prelude_complete_channel)
  result = run_next_code(expr, initial_line_number: repl_interface.line_number - repl_interface.lines.size - 1)
  # ...

That would remove the need to save the channel in an ivar and it could directly be passed as argument of check_prelude_complete.

That would be perfect after that, thanks! 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! Didn't connect that's where the yield was going back to, cleans up the code nicely!

@Vici37
Copy link
Contributor Author

Vici37 commented Jun 24, 2022

Updated! Also cleaned up the commit history a bit

@I3oris I3oris merged commit ba79faa into I3oris:master Jun 24, 2022
@I3oris
Copy link
Owner

I3oris commented Jun 24, 2022

Thank you Vici37! That's perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants