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

Support deferred expiration of records and attributes for one-to-many associations #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,27 @@ namespace :profile do
ruby "./performance/profile.rb"
end
end

namespace :db do
desc "Create the identity_cache_test database"
task :create do
require "mysql2"

config = {
host: ENV.fetch("MYSQL_HOST") || "localhost",
port: ENV.fetch("MYSQL_PORT") || 1037,
username: ENV.fetch("MYSQL_USER") || "root",
password: ENV.fetch("MYSQL_PASSWORD") || "",
}

begin
client = Mysql2::Client.new(config)
client.query("CREATE DATABASE IF NOT EXISTS identity_cache_test")
puts "Database 'identity_cache_test' created successfully. host: #{config[:host]}, port: #{config[:port]}"
rescue Mysql2::Error => e
puts "Error creating database: #{e.message}"
ensure
client&.close
end
end
end
16 changes: 10 additions & 6 deletions dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ up:
- bundler
- memcached
- mysql
- custom:
name: create database identity_cache_test
met?: mysql -u root -h 127.0.0.1 -P 1037 -e "SHOW DATABASES;" | grep identity_cache_test
meet: bundle exec rake db:create

commands:
test:
syntax:
optional:
argument: file
optional: args...
desc: 'Run tests'
desc: "Run tests"
run: |
if [[ $# -eq 0 ]]; then
bundle exec rake test
Expand All @@ -21,21 +25,21 @@ commands:
fi

style:
desc: 'Run rubocop checks'
desc: "Run rubocop checks"
run: bundle exec rubocop "$@"

check:
desc: 'Run tests and style checks'
desc: "Run tests and style checks"
run: bundle exec rake test && bundle exec rubocop

benchmark-cpu:
desc: 'Run the identity cache CPU benchmark'
desc: "Run the identity cache CPU benchmark"
run: bundle exec rake benchmark:cpu

profile:
desc: 'Profile IDC code'
desc: "Profile IDC code"
run: bundle exec rake profile:run

update-serialization-format:
desc: 'Update serialization format test fixture'
desc: "Update serialization format test fixture"
run: bundle exec rake update_serialization_format
20 changes: 20 additions & 0 deletions lib/identity_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class InverseAssociationError < StandardError; end

class NestedDeferredParentBlockError < StandardError; end

class NestedDeferredAttributeExpirationBlockError < StandardError; end

class UnsupportedScopeError < StandardError; end

class UnsupportedAssociationError < StandardError; end
Expand Down Expand Up @@ -240,6 +242,24 @@ def with_deferred_parent_expiration
Thread.current[:idc_parent_records_for_cache_expiry].clear
end

def with_deferred_attribute_expiration
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth creating a joint method alongside the parent expiry deferral? From the public API's perspective, it's a convenience method and I imagine most clients will want to do both. For our use case we do and it would save us having to nest one within the other!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be nice to collapse those two.

Maybe with_deferred_expiration that calls both with_deferred_parent_expiration and with_deferred_attribute_expiration.

Or with_deferred_expiration(parent: true, attribute: false as a way to control that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I am going to merge the two methods, and test again. 👍

raise NestedDeferredAttributeExpirationBlockError if Thread.current[:identity_cache_deferred_attribute_expiration]

Thread.current[:idc_deferred_attribute_expiration] = true
Thread.current[:idc_records_to_expire] = Set.new
Thread.current[:idc_attributes_to_expire] = Set.new

result = yield

Thread.current[:idc_deferred_attribute_expiration] = nil
IdentityCache.cache.delete_multi(Thread.current[:idc_records_to_expire])
IdentityCache.cache.delete_multi(Thread.current[:idc_attributes_to_expire])
result
ensure
Thread.current[:idc_deferred_attribute_expiration] = nil
Thread.current[:idc_attributes_to_expire].clear
end

def with_fetch_read_only_records(value = true)
old_value = Thread.current[:identity_cache_fetch_read_only_records]
Thread.current[:identity_cache_fetch_read_only_records] = value
Expand Down
9 changes: 9 additions & 0 deletions lib/identity_cache/cache_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ def delete(key)
@cache_backend.write(key, IdentityCache::DELETED, expires_in: IdentityCache::DELETED_TTL.seconds)
end

def delete_multi(keys)
drinkbeer marked this conversation as resolved.
Show resolved Hide resolved
key_values = keys.map { |key| [key, IdentityCache::DELETED] }.to_h
@cache_backend.write_multi(key_values, expires_in: IdentityCache::DELETED_TTL.seconds)
end

def clear
@cache_backend.clear
end
Expand All @@ -82,6 +87,10 @@ def fetch(key, fill_lock_duration: nil, lock_wait_tries: 2, &block)
end
end

def exist?(key)
@cache_backend.exist?(key)
end

private

def fetch_without_fill_lock(key)
Expand Down
18 changes: 15 additions & 3 deletions lib/identity_cache/cached/attribute.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,24 @@ def expire(record)

unless record.send(:was_new_record?)
old_key = old_cache_key(record)
all_deleted = IdentityCache.cache.delete(old_key)

if Thread.current[:idc_deferred_attribute_expiration]
Thread.current[:idc_attributes_to_expire] << old_key
# defer the deletion, and don't block the following deletion
all_deleted = true
else
all_deleted = IdentityCache.cache.delete(old_key)
end
end
unless record.destroyed?
new_key = new_cache_key(record)
if new_key != old_key
all_deleted = IdentityCache.cache.delete(new_key) && all_deleted
if Thread.current[:idc_deferred_attribute_expiration]
Thread.current[:idc_attributes_to_expire] << new_key
all_deleted = true
else
all_deleted = IdentityCache.cache.delete(new_key) && all_deleted
end
end
end

Expand Down Expand Up @@ -152,9 +164,9 @@ def new_cache_key(record)
end

def old_cache_key(record)
changes = record.transaction_changed_attributes
old_key_values = key_fields.map do |field|
field_string = field.to_s
changes = record.transaction_changed_attributes
if record.destroyed? && changes.key?(field_string)
changes[field_string]
elsif record.persisted? && changes.key?(field_string)
Expand Down
6 changes: 5 additions & 1 deletion lib/identity_cache/cached/primary_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,11 @@ def fetch_multi(ids)

def expire(id)
id = cast_id(id)
IdentityCache.cache.delete(cache_key(id))
if Thread.current[:idc_deferred_attribute_expiration]
Thread.current[:idc_records_to_expire] << cache_key(id)
else
IdentityCache.cache.delete(cache_key(id))
end
end

def cache_key(id)
Expand Down
14 changes: 14 additions & 0 deletions lib/identity_cache/memoized_cache_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ def delete(key)
end
end

def delete_multi(keys)
memoizing = memoizing?
ActiveSupport::Notifications.instrument("cache_delete_multi.identity_cache", memoizing: memoizing) do
if memoizing
keys.each { |key| memoized_key_values.delete(key) }
end
@cache_fetcher.delete_multi(keys)
end
end

def fetch(key, cache_fetcher_options = {}, &block)
memo_misses = 0
cache_misses = 0
Expand Down Expand Up @@ -137,6 +147,10 @@ def clear
end
end

def exist?(key)
@cache_fetcher.exist?(key)
end

private

EMPTY_ARRAY = [].freeze
Expand Down
44 changes: 44 additions & 0 deletions test/save_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,53 @@ def test_expire_cache_works_in_a_transaction
end
end

def test_touch_with_separate_calls
@record1 = Item.create(title: "fooz", created_at: 1.second.ago, updated_at: 1.second.ago)
@record2 = Item.create(title: "barz", created_at: 1.second.ago, updated_at: 1.second.ago)
id_and_title_key1 = "\"#{@record1.id}\"/\"fooz\""
expect_cache_delete("#{NAMESPACE}attr:Item:id:id/title:#{cache_hash(id_and_title_key1)}")
expect_cache_delete("#{NAMESPACE}attr:Item:id:title:#{cache_hash('"fooz"')}")
id_and_title_key2 = "\"#{@record2.id}\"/\"barz\""
expect_cache_delete("#{NAMESPACE}attr:Item:id:id/title:#{cache_hash(id_and_title_key2)}")
expect_cache_delete("#{NAMESPACE}attr:Item:id:title:#{cache_hash('"barz"')}")
expect_cache_delete(@record1.primary_cache_index_key)
expect_cache_delete(@record2.primary_cache_index_key)

ActiveRecord::Base.transaction do
@record1.touch
@record2.touch
end
end

def test_touch_with_batched_calls
@record1 = Item.create(title: "fooz", created_at: 1.second.ago, updated_at: 1.second.ago)
@record2 = Item.create(title: "barz", created_at: 1.second.ago, updated_at: 1.second.ago)
id_and_title_key1 = "\"#{@record1.id}\"/\"fooz\""
id_and_title_key2 = "\"#{@record2.id}\"/\"barz\""
expect_cache_deletes([
"#{NAMESPACE}attr:Item:id:title:#{cache_hash('"fooz"')}",
"#{NAMESPACE}attr:Item:id:id/title:#{cache_hash(id_and_title_key1)}",
"#{NAMESPACE}attr:Item:id:title:#{cache_hash('"barz"')}",
"#{NAMESPACE}attr:Item:id:id/title:#{cache_hash(id_and_title_key2)}",
])
expect_cache_deletes([@record1.primary_cache_index_key, @record2.primary_cache_index_key])

IdentityCache.with_deferred_attribute_expiration do
ActiveRecord::Base.transaction do
@record1.touch
@record2.touch
end
end
end

private

def expect_cache_delete(key)
@backend.expects(:write).with(key, IdentityCache::DELETED, anything)
end

def expect_cache_deletes(keys)
key_values = keys.map { |key| [key, IdentityCache::DELETED] }
@backend.expects(:write_multi).with(key_values, anything)
end
end
Loading