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

Make OutputBuffer more performant for 3.2+ YJIT #723

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelherold
Copy link
Contributor

This is a 🙋 feature or enhancement.

Summary

This change ports Jean Boussier's Rails change over to Bridgetown with a few tweaks.

  1. Using instance_of? against the constant is faster than the alternative self.class == other.class construct
  2. Linear, comparable logic for <<
  3. An expanded set of delegated methods to include #empty?, #encode, #lines, #reverse, #strip, and #valid_encoding? because these are all called in the test suite.

In the current test suite, this is a count of how many times each method is called:

2794 empty?
  54 encode
  54 encoding
 381 lines
 127 reverse
 127 strip
  54 valid_encoding?

It's unclear to me which, if any, of these should be public API so I kept all of the delegated methods as well as the ones Jean picked for Rails.

Context

Closes #599

This change ports Jean Boussier's Rails change over to Bridgetown with a
few tweaks.

1. Using `instance_of?` against the constant is faster than the
   alternative `self.class == other.class` construct
2. Linear, comparable logic for `<<`
3. An expanded set of delegated methods to include `#empty?`, `#encode`,
   `#lines`, `#reverse`, `#strip`, and `#valid_encoding?` because these
   are all called in the test suite.

In the current test suite, this is a count of how many times each method
is called:

2794 empty?
  54 encode
  54 encoding
 381 lines
 127 reverse
 127 strip
  54 valid_encoding?

It's unclear to me which, if any, of these should be public API so I
kept all of the delegated methods as well as the ones Jean picked for
Rails.
@sandstrom
Copy link
Contributor

sandstrom commented Apr 19, 2023

Sorry for the skepticism, but is it worth maintaining this extra code, over using ActiveSupport::SafeBuffer directly?

Rails render HTML for each HTTP request, but BridgeTown is a static site builder, building once during compilation (when no user is waiting, only the developer). In short, performance in the build step is less of an issue, generally speaking.

If we still think that this is an important change to make, would it be possible to import the ActionView::OutputBuffer class directly from action_view instead?

That way, we don't have to maintain this.

Just a thought though, feel free to disregard.

@michaelherold
Copy link
Contributor Author

That seems reasonable to me. This was me looking at the open issues list and finding one that I could do. I'm not sure why we don't currently use ActionView::OutputBuffer directly.

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

Successfully merging this pull request may close these issues.

Implement Ruby 3.2+ speedup for template output buffers
2 participants