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

Jump to bottom

Add Cached module to simplify caching code. #16671

Closed
wants to merge 1 commit into from
Closed

Add Cached module to simplify caching code. #16671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions Library/Homebrew/cask/cask_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,7 @@ def self.default_path(token)
def self.find_cask_in_tap(token, tap)
filename = "#{token}.rb"

Tap.cask_files_by_name(tap)
.fetch(token, tap.cask_dir/filename)
tap.cask_files_by_name.fetch(token, tap.cask_dir/filename)
end
end
end
2 changes: 1 addition & 1 deletion Library/Homebrew/diagnostic.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ def check_casktap_integrity
core_cask_tap = CoreCaskTap.instance
return unless core_cask_tap.installed?

broken_tap(core_cask_tap) || examine_git_origin(core_cask_tap.git_repo, core_cask_tap.remote)
broken_tap(core_cask_tap) || examine_git_origin(core_cask_tap.git_repo, T.must(core_cask_tap.remote))
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed to make this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually returning super in CoreTap#remote when HOMEBREW_NO_INSTALL_FROM_API is set.

end

sig { returns(T.nilable(String)) }
Expand Down
49 changes: 49 additions & 0 deletions Library/Homebrew/extend/cached.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# typed: strict
# frozen_string_literal: true

module Cached
module Clear
Copy link
Member

Choose a reason for hiding this comment

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

Why is a new module needed now? Is this only used once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is a new module needed now?

To make the code more readable and easier to maintain, i.e. not having to manually keep track of which cached variables need to be cleared.

Is this only used once?

So far only in Tap, but it should be possible to replace Cachable with this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be good to use this in 2+ places if it's added here to verify the API is sufficiently generic and better test the edges.

Copy link
Contributor

Choose a reason for hiding this comment

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

So far only in Tap, but it should be possible to replace Cachable with this.

I think having both Cachable and Cached is a bit confusing especially if they are used in the same module. The long-term goal should be to only have one. I prefer the simplicity of Cachable in most cases. Cached is not as flexible and that means once we try to cache methods that take arguments the interface is not as clean and it's possible that we might need to add helper methods which actually add complexity.

I like the approach and I think the code is cool but I'm not sure it's worth the cost here from a maintainability point of view.

As a quick comparison of Cachable and Cached, I'll show some examples of things that would be easier and things that would be harder.

# Now (on master)
def git_head
  raise TapUnavailableError, name unless installed?

  @git_head ||= git_repo.head_ref
end

# Cached
cached def git_head
  raise TapUnavailableError, name unless installed?

  git_repo.head_ref
end

# Cachable
def git_head
  raise TapUnavailableError, name unless installed?

  cache[:git_head] ||= git_repo.head_ref
end

These functions are all similar complexity. The advantage of caching is being able to clear the cache with one easy method which would be provided by both Cached and Cachable.

Note: Cached and Cachable are not exactly equivalent here. Cached will only be evaluated once while Cachable will check if the tap is installed each time though the head ref will be cached.

Here's another example.

# Now (on master)
def audit_exceptions
  @audit_exceptions ||= begin
    ensure_installed!
    super
  end
end

# Cached
cached def audit_exceptions
  ensure_installed!
  super
end

# Cachable
def audit_exceptions
  cache[:audit_exceptions] ||= begin
    ensure_installed!
    super
  end
end

The complexity is not much different in this case either. I would argue that it's also easier to understand at a glance since it uses basic Ruby patterns as well but that's very subjective.

One fine example, that's a bit different.

# Cachable (on master)
def self.fetch(user, repo)
  ...

  cache_key = "#{user}/#{repo}".downcase
  cache.fetch(cache_key) { |key| cache[key] = Tap.new(user, repo) }
end

# Cached
def self.fetch(user, repo)
  ...

  _fetch(user, repo)
end

private_class_method cached_class_method def self._fetch(user, repo)
  new(user, repo)
end

The Tap.fetch method shows that Cached is a not as flexible and in this case that means we need to create an additional method. One method is not the end of the world, of course, but it does suggest that this doesn't always simplify the code.

I feel like the simplest approach would just be to use Cachable without any modifications.

Copy link
Member

Choose a reason for hiding this comment

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

The long-term goal should be to only have one.

I agree with most of the above except: I don't think this should be "long-term", I would really like to avoid having both existing separately for any time/at all.

sig { void }
def clear_cache
super if defined?(super)

return unless defined?(@cached_method_calls)

remove_instance_variable(:@cached_method_calls)
end
end

sig { params(method: Symbol).returns(Symbol) }
def cached(method)
uncached_instance_method = instance_method(method)

define_method(method) do |*args, **options, &block|
@cached_method_calls ||= T.let({}, T.nilable(T::Hash[Symbol, T::Hash[T.untyped, T.untyped]]))
cache = @cached_method_calls[method] ||= {}

key = [args, options, block]
if cache.key?(key)
cache.fetch(key)
else
cache[key] = uncached_instance_method.bind(self).call(*args, **options, &block)
end
end
end

sig { params(method: Symbol).returns(Symbol) }
def cached_class_method(method)
uncached_singleton_method = singleton_method(method)

define_singleton_method(method) do |*args, **options, &block|
@cached_method_calls ||= T.let({}, T.nilable(T::Hash[Symbol, T::Hash[T.untyped, T.untyped]]))
cache = @cached_method_calls[method] ||= {}

key = [args, options, block]
if cache.key?(key)
cache[key]
else
cache[key] = uncached_singleton_method.call(*args, **options, &block)
end
end
end
Comment on lines +16 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it really makes sense for cached methods to take arguments so this can probably be simplified a bit more. None of the cache methods in Tap take arguments right now. At the very least, I think it's a bad idea to allow cached methods to take blocks. I don't think it's be terribly difficult to add tests to make sure that methods are defined that way too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm a bit surprised that you can use a block as a hash key. I guess I'd just never considered even attempting that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it really makes sense for cached methods to take arguments so this can probably be simplified a bit more. None of the cache methods in Tap take arguments right now. At the very least, I think it's a bad idea to allow cached methods to take blocks.

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it does make sense, for example Tap::_fetch will cache it based on user and repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

The block may not be needed though.

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this functionality unless it's needed in 3+ places.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a pattern in the code to create complex hash keys by concatenating strings with dashes. I'm not sure if this is preferable though to just using arrays of objects. Does anyone have strong feelings either way?

end
9 changes: 9 additions & 0 deletions Library/Homebrew/extend/cached.rbi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# typed: true

module Cached
requires_ancestor { Module }

module Clear
include Kernel
end
end
3 changes: 1 addition & 2 deletions Library/Homebrew/formulary.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,7 +1189,6 @@ def self.find_formula_in_tap(name, tap)
"#{name}.rb"
end

Tap.formula_files_by_name(tap)
.fetch(name, tap.formula_dir/filename)
tap.formula_files_by_name.fetch(name, tap.formula_dir/filename)
end
end