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 repeated allocations of node-type array #1452

Merged
merged 1 commit into from Jan 5, 2023
Merged

Avoid repeated allocations of node-type array #1452

merged 1 commit into from Jan 5, 2023

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: 186038173 bytes (1940980 objects)
    • Total retained: 4052257 bytes (53465 objects)
  • Savings
    • Total allocated: 1863585 bytes (46592 objects)
    • Total retained: -144 bytes (-2 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).

@@ -612,8 +612,9 @@ def comment_starts_line?(charno)
end

def insert_comments
node_types = [:comment, :void_stmt, :list]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does extracting this to a constant reduce allocations further?

Copy link
Owner

@lsegal lsegal Sep 3, 2022

Choose a reason for hiding this comment

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

I would imagine this to be true, especially if that list was then frozen.

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review. I am now using an array again.

@lsegal lsegal merged commit 2cb526a into lsegal:main Jan 5, 2023
@lsegal
Copy link
Owner

lsegal commented Jan 5, 2023

Getting around to these. Thanks for the contribution!

@ghost ghost deleted the perf/avoid-array-allocations branch March 22, 2023 01:05
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