8000 Replace OpenStruct with Struct by kyoshidajp · Pull Request #2004 · rack/rack · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Replace OpenStruct with Struct #2004

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2023
Merged

Replace OpenStruct with Struct #2004

merged 1 commit into from
Jan 8, 2023

Conversation

kyoshidajp
Copy link
Contributor

Using OpenStruct is not recommended from a performance or bug-prone standpoint.

Ref: https://docs.ruby-lang.org/en/3.2/OpenStruct.html#class-OpenStruct-label-Caveats

Performance

Add Frame class by Struct, and replace https://github.com/rack/rack/blob/v3.0.3/lib/rack/show_exceptions.rb#L81-L103 with the following code.

require "benchmark"
frames = nil
Benchmark.bm 10 do |r|
  r.report "OpenStruct" do
    frames = frames = exception.backtrace.map { |line|
      frame = OpenStruct.new
      if line =~ /(.*?):(\d+)(:in `(.*)')?/
        frame.filename = $1
        frame.lineno = $2.to_i
        frame.function = $4

        begin
          lineno = frame.lineno - 1
          lines = ::File.readlines(frame.filename)
          frame.pre_context_lineno = [lineno - CONTEXT, 0].max
          frame.pre_context = lines[frame.pre_context_lineno...lineno]
          frame.context_line = lines[lineno].chomp
          frame.post_context_lineno = [lineno + CONTEXT, lines.size].min
          frame.post_context = lines[lineno + 1..frame.post_context_lineno]
        rescue
        end

        frame
      else
        nil
      end
    }.compact
  end

  r.report "Struct" do
    frames = frames = exception.backtrace.map { |line|
      frame = Frame.new
      if line =~ /(.*?):(\d+)(:in `(.*)')?/
        frame.filename = $1
        frame.lineno = $2.to_i
        frame.function = $4

        begin
          lineno = frame.lineno - 1
          lines = ::File.readlines(frame.filename)
          frame.pre_context_lineno = [lineno - CONTEXT, 0].max
          frame.pre_context = lines[frame.pre_context_lineno...lineno]
          frame.context_line = lines[lineno].chomp
          frame.post_context_lineno = [lineno + CONTEXT, lines.size].min
          frame.post_context = lines[lineno + 1..frame.post_context_lineno]
        rescue
        end

        frame
      else
        nil
      end
    }.compact
  end
end

Add the following tests to test/spec_show_exceptions.rb, then run it.

  it "benchmark OpenStruct and Struct" do
    res = nil

    req = Rack::MockRequest.new(
      show_exceptions(
        lambda{|env| raise RuntimeError, "foo", ["nonexistent.rb:2:in `a': adf (RuntimeError)"] * 100 }
      ))

    res = req.get("/", "HTTP_ACCEPT" => "text/html")
  end

The result in my environment (Ruby 3.2.0 on macOS Big Sur), is the following.

# Running:

                 user     system      total        real
OpenStruct   0.001566   0.000113   0.001679 (  0.001678)
Struct       0.000351   0.000073   0.000424 (  0.000424)
.

Finished in 0.010989s, 91.0001 runs/s, 0.0000 assertions/s.

1 runs, 0 assertions, 0 failures, 0 errors, 0 skips

A backtrace of about 100 lines is not dramatically slow to begin with, but it is effective.

@ioquatix ioquatix merged commit c2fb4be into rack:main Jan 8, 2023
@ioquatix
Copy link
Member
ioquatix commented Jan 8, 2023

Thanks, this looks good to me.

@kyoshidajp kyoshidajp deleted the replace_openstruct_with_struct branch January 8, 2023 07:41
Earlopain added a commit to Earlopain/rack that referenced this pull request Apr 2, 2024
@Earlopain
Remove ostruct require
b574a44 
OpenStruct usage was removed in rack#2004 but this was missed.

In Ruby 3.5 this require would trigger a warning
@Earlopain Earlopain mentioned this pull request Apr 2, 2024
Merged
jeremyevans pushed a commit that referenced this pull request Apr 2, 2024
@Earlopain @jeremyevans
Remove ostruct require
40df0e2 
OpenStruct usage was removed in #2004 but this was missed.

In Ruby 3.5 this require would trigger a warning
@Earlopain Earlopain mentioned this pull request May 20, 2025
Merged
Earlopain pushed a commit to Earlopain/rack that referenced this pull request May 23, 2025
@kyoshidajp @Earlopain
Replace OpenStruct with Struct (rack#2004)
76dc38e 
To improve performance and specify fields explicitly.
Earlopain added a commit to Earlopain/rack that referenced this pull request May 23, 2025
@Earlopain
Remove ostruct require
5a9dedf 
OpenStruct usage was removed in rack#2004 but this was missed.

In Ruby 3.5 this require would trigger a warning
@Earlopain Earlopain mentioned this pull request May 23, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
54EC

@ioquatix ioquatix ioquatix approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kyoshidajp @ioquatix
0