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

Skip check for pending breakpoints if no breakpoints are present #738

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

WillHalto
Copy link
Contributor

@WillHalto WillHalto commented Aug 23, 2022

Description

Describe your changes:

  • Calls to ThreadClient.current.on_load spend time pushing and popping the :load operation on the event queue, and waiting for the :load event to be handled.
  • In a large Rails app with many loads, this extra time results a ~10-20% increase in environment load time when using require "debug", event when there are no breakpoints set at all. This seems to be at odd's with the README's "Fast: No performance penalty on non-stepping mode and non-breakpoints."
  • By skipping calls to ThreadClient.current.on_load when there are no breakpoints present at all, we can save some of this time.
  • As far as I can tell this is safe, as the main responsibility of ThreadClient.current.on_load is to handle any pending breakpoints in the loaded code, which we can safely skip if @bps is empty.

Example:

file_1.rb:

# file_1.rb
require "debug"

10000.times do
  load "./file_2.rb"
end

file_2.rb:

# file_2.rb
true

Before 949562d:

$ time bundle exec ruby file_1.rb

real    0m1.602s
user    0m1.044s
sys     0m0.690s

After 949562d:

$ time bundle exec ruby file_1.rb

real    0m0.835s
user    0m0.533s
sys     0m0.303s

^ It's a significant improvement in the overhead when using require "debug"

@st0012
Copy link
Member

st0012 commented Aug 23, 2022

The on_load event also stores loaded files into the SourceRepository, which will later be used to display frames' file_lines. So skipping them will likely cause list command to return empty result.

@WillHalto
Copy link
Contributor Author

The on_load event also stores loaded files into the SourceRepository, which will later be used to display frames' file_lines. So skipping them will likely cause list command to return empty result.

Gotcha, I tried this out locally but I wasn't able to see that behavior. list still works fine.
It appears that for Ruby versions 3.1 and later, SourceRepository#add doesn't save any source info. It just relies on iseq.script_lines to get the lines.

For versions prior to 3.1, SourceRepository#add does store file lines, but skipping that appears to not be an issue either, as SourceRepository#get will fall back to adding them at retrieval time if they're not already stored.

Does that seem right? Apologies if I am missing something!

@st0012
Copy link
Member

st0012 commented Aug 30, 2022

@WillHalto Yes I think you get it right. I missed the fallback mechanism.

@WillHalto
Copy link
Contributor Author

Yes I think you get it right. I missed the fallback mechanism.

@st0012 thanks for the feedback! Based on that ^, does the approach in 949562d seem alright?

Alternatively, if we do want to preserve the calls to on_load even when there are no breakpoints, I wonder if we could directly call Session#on_load in the tracepoint instead of calling it via the thread client. I noticed there's a decent amount of time spent dealing with the :load events via the event queue:

==================================
  Mode: wall(1000)
  Samples: 2676 (7.40% miss rate)
  GC: 220 (8.22%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
	 ...
      1355  (50.6%)         456  (17.0%)     Kernel#require
       221   (8.3%)         221   (8.3%)     Thread#join
       211   (7.9%)         211   (7.9%)     Thread::Queue#pop
       138   (5.2%)         138   (5.2%)     (marking)

which I don't believe is necessary. Perhaps we could instead try:

@tp_load_script = TracePoint.new(:script_compiled){|tp|
-    ThreadClient.current.on_load tp.instruction_sequence, tp.eval_script
+    on_load tp.instruction_sequence, tp.eval_script
}

and skip the event queue.

This works well in my tests and gives a similar (+/- ~100ms) performance improvement as the current proposed approach in the PR. Just something else to consider 👍

Before:

$ time bundle exec ruby file_1.rb

real    0m1.602s
user    0m1.044s
sys     0m0.690s

After change in 949562d

$ time bundle exec ruby file_1.rb

real    0m0.835s
user    0m0.533s
sys     0m0.303s

Directly calling on_load, skipping event queue:

time bundle exec ruby file_1.rb

real    0m0.998s
user    0m0.665s
sys     0m0.332s

@ko1
Copy link
Collaborator

ko1 commented Oct 26, 2022

Thank you for the idea.

  • Before Ruby 3.1 source code repository is needed for eval'ed code and modified code (the code modified after loading).
  • After Ruby 3.1, yes we can skip.

So I think we can skip it on Ruby 3.1 and later. Do you modify this PR?

@WillHalto WillHalto force-pushed the willhalto/faster-require branch from 949562d to 0e12fa1 Compare October 28, 2022 20:11
@WillHalto
Copy link
Contributor Author

@ko1 I've updated this in 0e12fa1 to only skip it on Ruby 3.1 and later 👍

@ko1 ko1 merged commit ad1bcb1 into ruby:master Nov 2, 2022
@ko1
Copy link
Collaborator

ko1 commented Nov 2, 2022

Thank you.

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

Successfully merging this pull request may close these issues.

3 participants