-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Improve perf of ActionView::Helpers::TextHelper#excerpt
for large strings.
#39979
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
Conversation
63b25ce
to
ae87891
Compare
@@ -467,18 +467,25 @@ def cut_excerpt_part(part_position, part, separator, options) | |||
radius = options.fetch(:radius, 100) | |||
omission = options.fetch(:omission, "...") | |||
|
|||
part = part.split(separator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very expensive when separator
is ""
, we end up generating a giant array of characters for a large string.
ae87891
to
0460290
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ✂️ the benchmark from the commit message? 10,000 words of lorem ipsum is a bit much 😅
else | ||
part.first(radius) | ||
affix = part.length > radius ? omission : "" | ||
part = part.public_send(part_position == :first ? :last : :first, radius) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complex enough that I prefer the longer version from your benchmark script:
part = part.public_send(part_position == :first ? :last : :first, radius) | |
part = | |
if part_position == :first | |
part.last(radius) | |
else | |
part.first(radius) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to require active_support/core_ext/string/access
in order to use String#first
and String#last
here.
The pull request title and commit message both mention |
0460290
to
86a264b
Compare
ActionView::Helpers::TextHelper#truncate
for large strings.ActionView::Helpers::TextHelper#excerpt
for large strings.
86a264b
to
a40d3de
Compare
@eugeneius Sorry for the late reply as I got caught up with a project at work. I've updated the PR and commit as per your review. |
Thanks @tgxworld! |
Benchmark Script
Results: