Avoid repeated allocations of node-type array #1452
Conversation
lib/yard/parser/ruby/ruby_parser.rb
Outdated
| @@ -612,8 +612,9 @@ def comment_starts_line?(charno) | |||
| end | |||
|
|
|||
| def insert_comments | |||
| node_types = [:comment, :void_stmt, :list] | |||
There was a problem hiding this comment.
Does extracting this to a constant reduce allocations further?
There was a problem hiding this comment.
I would imagine this to be true, especially if that list was then frozen.
There was a problem hiding this comment.
Thanks for the suggestion. I implemented the change and reduced the number of allocations by ~80 or so.
I also switched the container to a Set to avoid repeated O(n) look-ups.
| @@ -613,7 +620,7 @@ def comment_starts_line?(charno) | |||
|
|
|||
| def insert_comments | |||
| root.traverse do |node| | |||
| next if [:comment, :void_stmt, :list].include?(node.type) || node.parent.type != :list | |||
| next if COMMENT_SKIP_NODE_TYPES.include?(node.type) || node.parent.type != :list | |||
There was a problem hiding this comment.
Because of how Set is implemented in MRI, the array implementation is likely to be faster at this scale. (set is also not a prior dependency of yard, although that is moot as of Ruby 3.2)
There was a problem hiding this comment.
Thanks for the review. I am now using an array again.
|
Getting around to these. Thanks for the contribution! |
ghost
deleted the
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
rvmon Linux)ruby-memory-profiler --no-color --retained-strings=500 --allocated-strings=500 --max=300 -o prof.log ./run-yard.rb statsrun-yard.rbis a smaller version of/bin/yard(MemoryProfiler was not able to run/bin/yard)statscommand to avoid memory exhaustionPerformance data
Completed Tasks
bundle exec rakelocally (if code is attached to PR).