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

Avoid regex allocations by checking arity condition first #1455

Merged
merged 1 commit into from Sep 3, 2022
Merged

Avoid regex allocations by checking arity condition first #1455

merged 1 commit into from Sep 3, 2022

Conversation

ghost
Copy link

@ghost ghost commented Aug 18, 2022

Description

Hi, I am trying to optimize YARD a bit to speed up documentation runs on a large codebase.

The codebase is private, so I am using https://github.com/watir/watir/ as a smaller stand-in to report benchmarking numbers from https://github.com/SamSaffron/memory_profiler.

Test set-up

  • Ruby 2.7.6 (installed via rvm on Linux)
  • Watir version: 293bed2b57c
  • MemoryProfiler version: 1.0.0
  • YARD baseline: 0a55093
  • Test command:
    • ruby-memory-profiler --no-color --retained-strings=500 --allocated-strings=500 --max=300 -o prof.log ./run-yard.rb stats
      • run-yard.rb is a smaller version of /bin/yard (MemoryProfiler was not able to run /bin/yard)
      • I am using the stats command to avoid memory exhaustion

Performance data

  • Before
    • Total allocated: 187901758 bytes (1987572 objects)
    • Total retained: 4052113 bytes (53463 objects)
  • After
    • Total allocated: 187895022 bytes (1987442 objects)
    • Total retained: 4052113 bytes (53463 objects)
  • Savings
    • Total allocated: 6736 bytes (130 objects)
    • Total retained: 0 bytes (0 objects)

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@@ -144,7 +144,7 @@ def file_encoding
PARSER_EVENT_TABLE.each do |event, arity|
node_class = AstNode.node_class_for(event)

if /_new\z/ =~ event.to_s && arity == 0
if arity == 0 && /_new\z/ =~ event.to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the regex check be replaced with event.to_s.end_with?('_new')

Copy link
Owner

Choose a reason for hiding this comment

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

arity is better since ends_with breaks prior Ruby compatibility.

Copy link
Contributor

@dduugg dduugg Aug 18, 2022

Choose a reason for hiding this comment

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

What’s the minimum supported ruby version? The gemspec says it depends on webrick 1.7.0, which requires ruby >= 2.3.0, which supports String#end_with?

edit: confirmed yard installation fails in ruby 2.2.10:

$ gem install yard                                                                                                                                                    17:39
Fetching: webrick-1.7.0.gem (100%)
ERROR:  Error installing yard:
	webrick requires Ruby version >= 2.3.0.

Copy link
Author

Choose a reason for hiding this comment

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

CONTRIBUTING.md says:

Please also remember that YARD supports a number of environments, including OS X, Linux, Windows, and a number of older Ruby versions (1.8+)

If the minimum version is no longer 1.8, then some of the =~ checks can indeed be replaced with less expensive end_with? and start_with? checks.

Copy link
Owner

Choose a reason for hiding this comment

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

Good to know that end_with is available, but a little moot since arity is already here :)

@lsegal lsegal merged commit e167846 into lsegal:main Sep 3, 2022
@lsegal
Copy link
Owner

lsegal commented Sep 3, 2022

Great change, thanks!

@ghost ghost deleted the perf/avoid-more-regexes branch September 12, 2022 16:35
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.

None yet

2 participants