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 double name resolution #2127

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Avoid double name resolution #2127

merged 2 commits into from
Dec 10, 2024

Conversation

ksss
Copy link
Collaborator

@ksss ksss commented Dec 5, 2024

The rbs validate command does double name resolution.
The env that executed resolve_type_names should have completed all name resolution, so name resolution within the validator should not be necessary.

Repro

rbs 1a3ab85
ruby 3.3.6

# Gemfile
source "https://rubygems.org"
gem 'rbs'
gem 'memory_profiler'
gem 'aws-sdk-s3'
gem 'aws-sdk-sqs'
gem 'aws-sdk-ec2'
gem 'aws-sdk-quicksight'
$ bundle install
$ bundle exec rbs collection init
$ bundle exec rbs collection install
# t.rb
require 'rbs'
require 'rbs/cli'
require 'memory_profiler'

options = RBS::CLI::LibraryOptions.new
validate = RBS::CLI::Validate.new(args: [], options: options)
report = MemoryProfiler.report do
  validate.run
end
report.pretty_print

Before

$ time bundle exec rbs validate
bundle exec rbs validate  9.95s user 0.44s system 98% cpu 10.505 total
$ time bundle exec ruby t.rb
Total allocated: 1434079736 bytes (13515513 objects)
Total retained:  239405736 bytes (2371893 objects)
...
allocated memory by location
-----------------------------------
 118300680  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:687
 115440840  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:374
  87703808  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/definition.rb:301
  71837200  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:603
  62730720  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:368
  46641112  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:604
  46364040  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:661

After

$ time bundle exec rbs validate
bundle exec rbs validate  7.16s user 0.33s system 97% cpu 7.690 total
$ time bundle exec ruby t.rb
Total allocated: 947277720 bytes (8836729 objects)
Total retained:  239405736 bytes (2371893 objects)
...
allocated memory by location
-----------------------------------
  87703808  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/definition.rb:301
  62730720  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:368
  46364040  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:661
  35529720  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/definition_builder.rb:323
  25566480  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/namespace.rb:90
  25244000  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:571
  23562000  /Users/yuki.kurihara/src/github.com/ksss/rbs/lib/rbs/types.rb:1085

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

👍

@soutaro soutaro added this pull request to the merge queue Dec 10, 2024
@soutaro soutaro added this to the RBS 3.8 milestone Dec 10, 2024
Merged via the queue into ruby:master with commit a0f37a9 Dec 10, 2024
18 checks passed
@ksss ksss deleted the mem-validate branch December 10, 2024 06:13
@soutaro soutaro added the Released PRs already included in the released version label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

2 participants