RBS scan cache to improve overall performance - #1796

Closed

skatkov wants to merge 8 commits into

ruby:masterfrom

skatkov:perf/rbs-member-lookups

skatkov commented Aug 29, 2026 •

edited

Loading

Contributor

RBS parsing repeatedly searched growing method and attribute arrays, making classes with many declarations quadratic. This replaces those searches with parser-local Hash indexes while leaving the ordered code-object collections unchanged.

The indexes are initialized from existing context members and updated when methods, attributes, or aliases are added. This preserves behavior for rebuilt stores, forward aliases, and legacy instance methods with a nil singleton value.

Five measured runs with 2,000 declarations per workload:

Parser-only workload	Baseline	Optimized	Speedup
RBS methods	0.1610s	0.0353s	4.6x
RBS attributes	0.1108s	0.0253s	4.4x

End-to-end workload	Baseline	Optimized	Change
RBS methods	0.722s	0.587s	-18.7%
RBS attributes	0.630s	0.569s	-9.8%

Benchmark
https://gist.github.com/skatkov/82f89eaf2269286a42a4adf966d2fd12

skatkov requested a deployment to fork-preview-protection

August 29, 2026 15:02

— with

GitHub Actions Waiting

skatkov marked this pull request as ready for review

August 29, 2026 15:43

Copilot AI balanced review requested due to automatic review settings

August 29, 2026 15:43

Copilot AI reviewed

Copilot AI left a comment

Contributor

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings

August 30, 2026 12:21

skatkov requested a deployment to fork-preview-protection

August 30, 2026 12:21

— with

GitHub Actions Waiting

Copilot AI reviewed

Copilot AI left a comment

Contributor

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings

August 30, 2026 12:44

skatkov requested a deployment to fork-preview-protection

August 30, 2026 12:44

— with

GitHub Actions Waiting

Copilot AI reviewed

Copilot AI left a comment

Contributor

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings

August 30, 2026 12:54

skatkov requested a deployment to fork-preview-protection

August 30, 2026 12:54

— with

GitHub Actions Waiting

Copilot AI reviewed

Copilot AI left a comment

Contributor

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings

August 30, 2026 14:17

skatkov requested a deployment to fork-preview-protection

August 30, 2026 14:17

— with

GitHub Actions Waiting

Copilot AI reviewed

Copilot AI left a comment

Contributor

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

tompng mentioned this pull request

Rename #initialize to ::new before registering it to the container #1802

Merged

tompng commented Aug 30, 2026

Member

I agree that the quadratic cost should be fixed. However, it also happens in the normal RDoc parse path, not just RBS.

It would be simpler to implement the cache in Context itself rather than in each parser. Making context.find_method faster fixes all of them at once, and the parser-side change in this PR reduces to a few lines.

Context already has a hash-based lookup: @methods_hash. It could not be used for find_method as-is because @method_list and @methods_hash could disagree — the main source was the #initialize → ::new rename performed after add_method, which left the hash keyed by the old name. I've just opened #1802 to remove that. Once it is merged, a parse-time lookup can read @methods_hash directly.

Concretely, we could add internal methods like context.find_method_from_hash / context.find_attribute_from_hash, marked # :nodoc:. Two things to keep in mind:

@methods_hash also holds attributes (for redefinition checks), so the method finder needs an RDoc::Attr check.
The remaining list/hash divergences (e.g. remove_invisible only removes from @method_list) happen after parsing, so they don't affect parse-time lookups like this PR's, but they are why the public find_method can't simply switch to the hash yet. Longer term, @method_list and @methods_hash should be unified into a single source of truth (same for @constants and @constants_hash), and these internal methods can be removed at that point.

skatkov marked this pull request as draft

August 30, 2026 19:12

skatkov commented Aug 30, 2026

Contributor Author

I will revisit this PR once #1802 will be merged.

tompng added a commit that referenced this pull request


          Rename #initialize to ::new before registering it to the container (

df14870

#1802)

## Background

The Ruby parser renames an instance method `initialize` to `::new` for
documentation purposes. This rename happened *after*
`container.add_method`, with a comment claiming the ordering is
intentional: "Rename after add_method to register duplicated 'new' and
'initialize' defined in c and ruby".

The actual reason for this placement is older. In the Ripper-based
streaming parser, documentation modifiers such as `:notnew:` were read
*after* the method line, so at `add_method` time the parser simply did
not know yet whether the method should be renamed:

```ruby
# Having now read the method parameters and documentation modifiers, we
# now know whether we have to rename #initialize to ::new
```
(lib/rdoc/parser/ripper_ruby.rb, removed in #1690)

The Prism parser processes directives and modifier lines before
`add_method`, so this constraint is gone. The post-add placement was a
consequence of the streaming parser's information ordering, not a design
goal — which is why it is safe to retire the post-add mutation pattern
now. The comment in the current code was a port-time rationalization of
an observable side effect.

## What the old placement actually did

Registering the method under the `#initialize` key and renaming it
afterwards had two effects:

1. `Context#methods_hash` was left keyed by a stale name (`#initialize`
pointing at a method now called `::new`). This is one of the obstacles
to making `Context#find_method` hash-based (see the discussion in
#1796).
2. Duplicate detection in `Context#add_method` was bypassed. A class
documenting both `::new` (an explicit `def self.new`, or a C-defined
`new`) and `#initialize` ended up with two `::new` entries on its page.

## Change

Move the rename block before `container.add_method`. All information it
uses (the method name, `singleton`, and `dont_rename_initialize` set by
the `:notnew:` directive) is already available at that point.

With the rename in place before registration, the normal deduplication
applies: the first registration wins, and the duplicate is reported by
the existing "Duplicate method" warning (visible with `--verbose`).

## Corpus diff

Compared per-class method lists (name, singleton, visibility, file) for
whole corpora, before vs after:

| Corpus | Changes |
| --- | --- |
| ruby/ruby | 4 classes lose a duplicated `::new` entry:
`Gem::Package::TarReader`, `Gem::Package::TarWriter`,
`Gem::Resolver::APISpecification`, `JSON::Ext::Generator::State` |
| activesupport 8.1.3 | 1 class:
`ActiveSupport::Deprecation::DeprecatedConstantProxy` |
| rdoc itself | no change |

All other entries are identical. Each changed class defines both `def
self.new` and `def initialize` (or, for `JSON::Ext::Generator::State`, a
C-defined `new` and a Ruby `initialize` — the exact "c and ruby" case
the old comment referred to). On master these pages show `new` twice,
with a duplicated `id="method-c-new"` anchor (invalid HTML; the index
can only link to the first entry); with this change they show it once.

## Cleanup

The second commit removes the `dont_rename_initialize` keyword argument
of `internal_add_method`. It has never been passed a truthy value since
its introduction: one call site passes an explicit `false` and the other
relies on the `false` default. It is unrelated to
`AnyMethod#dont_rename_initialize` (set by the `:notnew:` directive),
which remains the live mechanism; removing the constant-false parameter
also removes the confusion of two same-named flags in one method.

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

skatkov added 8 commits

September 1, 2026 18:16


          RBS cache during scan

a31e1e7


          Lazily update RBS member indexes

3b57817

Context#add_alias can append methods or attributes as a side effect, making
mutation-side cache updates depend on Context internals. Index each
collection's unconsumed tail during lookup instead, keeping synchronization
in one place while preserving linear lookup.


          remove method mocking tests

88028c6


          Revived both RBS linear-performance tests using assert_linear_perform…

7e63a45

…ance


          remove finder methods

558f2cc


          simplify singleton setup

78a6b4e


          combine two tests into one

e7f3e01


          Use index from Context

3c1659f

skatkov force-pushed the perf/rbs-member-lookups branch from 5ab0b8b to 3c1659f Compare

September 1, 2026 17:19

github-actions Bot commented Sep 1, 2026

Documentation preview

View the preview

Commit: 3c1659f

skatkov commented Sep 1, 2026

Contributor Author

This change show nice results on micro-benchmarks (x5 better), but with end-to-end testing of entire documentation generation workflow improvement is around 1-2% (within run-to-run noise).

It doesn't seem like Context hashes bring any real performance gain here. Nor does this bring any real memory improvements either.

@tompng I pushed my latest code changes, but I don't think it's worth it to continue. I'll be closing this PR.

If you still want these changes, we can reconsider.

skatkov closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Labels

None yet