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

Improve performance of show_source for large class #249

Conversation

pocke
Copy link
Member

@pocke pocke commented Jun 25, 2021

This patch improves performance of show_source command for large class.

Problem

show_source is slow for large class.
For example, show_source 'IRB::Irb' is slow. It takes 3.6s in my environment.

Because RubyLex.ripper_lex_without_warning is heavy but it is called on each line.

Profiling

$LOAD_PATH.unshift './lib/'
require 'irb'
require 'irb/cmd/show_source'

require 'stackprof'

IRB.setup('')
w = IRB::WorkSpace.new(binding)
irb = IRB::Irb.new(w)
ctx = IRB::Context.new(irb, w)


StackProf.run(mode: :wall, out: 'stackprof-out', raw: true) do
  IRB::ExtendCommand::ShowSource.new(ctx).execute('IRB::Irb')
end

210625154610

Solution

Call RubyLex.ripper_lex_without_warning just once, instead of calling it on each line.

This change also stops calling RubyLex#check-state, but it uses RubyLex#process_continue and check_code_block directly. Because the other checks in check_state methods, such as process_literal_type, are slow. This change makes 5x difference the speed.

Benchmark

code

$LOAD_PATH.unshift './lib/'
require 'irb'
require 'irb/cmd/show_source'

IRB.setup('')
w = IRB::WorkSpace.new(binding)
irb = IRB::Irb.new(w)
ctx = IRB::Context.new(irb, w)


module IRB
  module ExtendCommand
    class ShowSource < Nop
      def puts(*)
        # to suppress printing code
      end
    end
  end
end

require 'benchmark'
Benchmark.bmbm(20) do |x|
  x.report{10.times{
    IRB::ExtendCommand::ShowSource.new(ctx).execute('IRB::Irb')
  }}
end

result (before)

Rehearsal --------------------------------------------------------
                      36.009193   0.002964  36.012157 ( 36.056616)
---------------------------------------------- total: 36.012157sec

                           user     system      total        real
                      36.069129   0.000000  36.069129 ( 36.113850)

result (after)

Switch to inspect mode.
Switch to inspect mode.
Rehearsal --------------------------------------------------------
                       1.342156   0.000415   1.342571 (  1.344645)
----------------------------------------------- total: 1.342571sec

                           user     system      total        real
                       1.366725   0.003345   1.370070 (  1.372135)

In this case, it will be 26.3x faster 🚀

Comment on lines +64 to +68
prev_tokens = []

# chunk with line number
tokens.chunk { |tok| tok[0][0] }.each do |lnum, chunk|
code = lines[0..lnum].join
Copy link
Member

@k0kubun k0kubun Jun 27, 2021

Choose a reason for hiding this comment

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

Suggested change
prev_tokens = []
# chunk with line number
tokens.chunk { |tok| tok[0][0] }.each do |lnum, chunk|
code = lines[0..lnum].join
code = +""
prev_tokens = []
# chunk with line number
tokens.chunk { |tok| tok[0][0] }.each do |lnum, chunk|
code << lines[lnum]

You changed the way code is constructed, but this seems like a new bottleneck in your code. The original code seems optimal.

before

ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
Rehearsal --------------------------------------------------------
                       2.401019   0.011546   2.412565 (  2.414266)
----------------------------------------------- total: 2.412565sec

                           user     system      total        real
                       2.420137   0.005364   2.425501 (  2.426264)

after

ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]
Rehearsal --------------------------------------------------------
                       1.028879   0.009408   1.038287 (  1.039615)
----------------------------------------------- total: 1.038287sec

                           user     system      total        real
                       1.000221   0.007454   1.007675 (  1.008295)

Copy link
Member

@k0kubun k0kubun Jun 27, 2021

Choose a reason for hiding this comment

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

(The code I suggested might skip lines that don't have any token, but I assume such lines don't impact compilation error checks. Such lines seem to have an on_ignored_nl token.)

Copy link
Member

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

👍

@k0kubun k0kubun merged commit 1f82e2f into ruby:master Jun 27, 2021
k0kubun added a commit that referenced this pull request Jun 27, 2021
#249 actually slowed down how `code` is
concatenated. The original way of creating `code` is faster.

[before]
    user     system      total        real
2.420137   0.005364   2.425501 (  2.426264)

[after]
    user     system      total        real
1.000221   0.007454   1.007675 (  1.008295)

Theoretically, this implementation might skip lines that don't appear in
Ripper tokens, but this assumes such lines don't impact whether the code
passes compilation or not. At least normal blank lines seem to have an
`on_ignored_nl` token anyway though.
matzbot pushed a commit to ruby/ruby that referenced this pull request Jun 27, 2021
ruby/irb#249 actually slowed down how `code` is
concatenated. The original way of creating `code` is faster.

[before]
    user     system      total        real
2.420137   0.005364   2.425501 (  2.426264)

[after]
    user     system      total        real
1.000221   0.007454   1.007675 (  1.008295)

Theoretically, this implementation might skip lines that don't appear in
Ripper tokens, but this assumes such lines don't impact whether the code
passes compilation or not. At least normal blank lines seem to have an
`on_ignored_nl` token anyway though.

ruby/irb@27dd2867cd
@pocke pocke deleted the Improve_performance_of__show_source__for_large_class branch June 27, 2021 02:34
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.

None yet

2 participants