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.

Sign up for GitHub

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

Optimize URLHelpers.gather_constants #1935

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Optimize URLHelpers.gather_constants #1935

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70d7fb5
Optimize `URLHelpers.gather_constants`
amomchilov Jun 19, 2024
ef5e6c9
Flip subtype checks to remove edge case
amomchilov Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions lib/tapioca/dsl/compilers/url_helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,28 @@ def gather_constants
routes_reloader = Rails.application.routes_reloader
routes_reloader.execute_unless_loaded if routes_reloader&.respond_to?(:execute_unless_loaded)

Object.const_set(:GeneratedUrlHelpersModule, Rails.application.routes.named_routes.url_helpers_module)
Object.const_set(:GeneratedPathHelpersModule, Rails.application.routes.named_routes.path_helpers_module)
url_helpers_module = Rails.application.routes.named_routes.url_helpers_module
path_helpers_module = Rails.application.routes.named_routes.path_helpers_module
Comment on lines +109 to +110
Copy link
Contributor Author

@amomchilov amomchilov Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Extracting out these local variables removes the need to do repeated constant look-ups in the hot path below.


Object.const_set(:GeneratedUrlHelpersModule, url_helpers_module)
Object.const_set(:GeneratedPathHelpersModule, path_helpers_module)

constants = all_modules.select do |mod|
next unless name_of(mod)

includes_helper?(mod, GeneratedUrlHelpersModule) ||
includes_helper?(mod, GeneratedPathHelpersModule) ||
includes_helper?(mod.singleton_class, GeneratedUrlHelpersModule) ||
includes_helper?(mod.singleton_class, GeneratedPathHelpersModule)
# Fast-path to quickly disqualify most cases
next false unless url_helpers_module > mod || # rubocop:disable Style/InvertibleUnlessCondition
path_helpers_module > mod ||
url_helpers_module > mod.singleton_class ||
path_helpers_module > mod.singleton_class

includes_helper?(mod, url_helpers_module) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observation: this computes and traverses the ancestor chain for mod and mod.singleton_class twice each.

I tried doing it just once, like so:

mod_direct_ancestors = direct_ancestors_of(mod)

next true if mod_direct_ancestors.include?(url_helpers_module) ||
  mod_direct_ancestors.include?(path_helpers_module)
              
mod_meta_direct_ancestors = direct_ancestors_of(mod.singleton_class)
              
mod_meta_direct_ancestors.include?(url_helpers_module) ||
  mod_meta_direct_ancestors.include?(path_helpers_module)

using:

sig { params(mod: Module).returns(T::Array[Module]) }
private def direct_ancestors_of(mod)
  if Class === mod
    superclass = superclass_of(mod)
    ancestors_of(mod).take_while { |a| !superclass.equal?(a) }
  else
    ancestors_of(mod)
  end
end

But it turned out to be roughly 25% slower

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a bunch of methods returning arrays in https://github.com/Shopify/tapioca/blob/35a6a43b5469d5b03d292330449fd7b6db7593a6/lib/tapioca/runtime/reflection.rb that could benefit from returning sets instead.

includes_helper?(mod, path_helpers_module) ||
includes_helper?(mod.singleton_class, url_helpers_module) ||
includes_helper?(mod.singleton_class, path_helpers_module)
end

constants.concat(NON_DISCOVERABLE_INCLUDERS)
constants.concat(NON_DISCOVERABLE_INCLUDERS).push(GeneratedUrlHelpersModule, GeneratedPathHelpersModule)
end

sig { returns(T::Array[Module]) }
Expand All @@ -134,17 +143,20 @@ def gather_non_discoverable_includers
end.freeze
end

# Returns `true` if `mod` "directly" includes `helper`.
# For classes, this method will return false if the `helper` is included only by a superclass
sig { params(mod: Module, helper: Module).returns(T::Boolean) }
private def includes_helper?(mod, helper)
superclass_ancestors = []
ancestors = ancestors_of(mod)

if Class === mod
superclass = superclass_of(mod)
superclass_ancestors = ancestors_of(superclass) if superclass
own_ancestors = if Class === mod && (superclass = superclass_of(mod))
# These ancestors are unique to `mod`, and exclude those in common with `superclass`.
ancestors.take(ancestors.count - ancestors_of(superclass).size)
else
ancestors
end

ancestors = Set.new.compare_by_identity.merge(ancestors_of(mod)).subtract(superclass_ancestors)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The allocations and set operations here where the majority of the hot path.

ancestors.any? { |ancestor| helper == ancestor }
paracycle marked this conversation as resolved.
Show resolved Hide resolved
own_ancestors.include?(helper)
end
end

Expand Down
Loading