From b6e10ee8fef5d7df41b375306f02800bbfae18aa Mon Sep 17 00:00:00 2001 From: David Dawson Date: Sat, 20 Jul 2024 14:20:29 +0100 Subject: [PATCH 1/4] Reduce the retained memory when defining the relation readers Fixes: https://github.com/rom-rb/rom/issues/694 --- .../lib/rom/plugins/relation/registry_reader.rb | 17 ++--------------- .../rom/setup/finalize/finalize_relations.rb | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/core/lib/rom/plugins/relation/registry_reader.rb b/core/lib/rom/plugins/relation/registry_reader.rb index 28c754f08..fec206387 100644 --- a/core/lib/rom/plugins/relation/registry_reader.rb +++ b/core/lib/rom/plugins/relation/registry_reader.rb @@ -14,13 +14,9 @@ class RegistryReader < ::Module EMPTY_REGISTRY = RelationRegistry.build(EMPTY_HASH).freeze # @api private - attr_reader :relations - - # @api private - def initialize(relations:) + def initialize(klass:, relation_readers_module:) super() - @relations = relations - define_readers! + klass.include relation_readers_module end # @api private @@ -30,15 +26,6 @@ def included(klass) klass.option :__registry__, default: -> { EMPTY_REGISTRY } end - - private - - # @api private - def define_readers! - relations.each do |name| - define_method(name) { __registry__[name] } - end - end end end end diff --git a/core/lib/rom/setup/finalize/finalize_relations.rb b/core/lib/rom/setup/finalize/finalize_relations.rb index e0b73656e..056d4ab34 100644 --- a/core/lib/rom/setup/finalize/finalize_relations.rb +++ b/core/lib/rom/setup/finalize/finalize_relations.rb @@ -9,6 +9,18 @@ class Finalize class FinalizeRelations attr_reader :notifications + module BuildRelationReaders + def self.build(relations) + Module.new do + relations.each do |name| + define_method(name) do + __registry__[name] + end + end + end + end + end + # Build relation registry of specified descendant classes # # This is used by the setup @@ -32,6 +44,7 @@ def initialize(gateways, relation_classes, notifications:, mappers: nil, plugins # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def run! relation_registry = RelationRegistry.new do |registry, relations| + relation_readers_module = BuildRelationReaders.build(relation_names) @relation_classes.each do |klass| unless klass.adapter raise MissingAdapterIdentifierError, @@ -45,7 +58,7 @@ def run! "Relation with name #{key.inspect} registered more than once" end - klass.use(:registry_reader, relations: relation_names) + klass.use(:registry_reader, klass: klass, relation_readers_module: relation_readers_module) notifications.trigger( 'configuration.relations.class.ready', From efd8da6ca1343203b9aa6335257d557647d0fe2c Mon Sep 17 00:00:00 2001 From: David Dawson Date: Mon, 22 Jul 2024 16:10:17 +0100 Subject: [PATCH 2/4] Optimise the repository relations reader --- .../lib/rom/repository/relation_reader.rb | 43 +++++++++++++++---- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/repository/lib/rom/repository/relation_reader.rb b/repository/lib/rom/repository/relation_reader.rb index 94359c45d..d365471ea 100644 --- a/repository/lib/rom/repository/relation_reader.rb +++ b/repository/lib/rom/repository/relation_reader.rb @@ -4,28 +4,53 @@ module ROM class Repository # @api private class RelationReader < ::Module + extend ::Dry::Core::ClassAttributes + # @api private attr_reader :klass # @api private attr_reader :relations + defines :relation_readers + + defines :mutex + mutex(Mutex.new) + + defines :relation_cache + relation_cache(Concurrent::Hash.new) + module InstanceMethods # @api private def set_relation(name) # rubocop:disable Naming/AccessorMethodName container .relations[name] - .with(auto_struct: auto_struct) - .struct_namespace(struct_namespace) + .with(auto_struct: auto_struct, struct_namespace: struct_namespace) + end + + def relation_reader(name, relation_cache) + key = [name, auto_struct, struct_namespace] + relation_cache[key] ||= set_relation(name) end end + # @api private + def mutex + ROM::Repository::RelationReader.mutex + end + # @api private def initialize(klass, relations) super() - @klass = klass @relations = relations - define_readers! + mutex.synchronize do + unless self.class.relation_readers + self.class.relation_readers( + build_relation_readers(relations, self.class.relation_cache) + ) + end + end + klass.include self.class.relation_readers end # @api private @@ -37,10 +62,12 @@ def included(klass) private # @api private - def define_readers! - relations.each do |name| - define_method(name) do - @relations[name] ||= set_relation(name) + def build_relation_readers(relations, relation_cache) + Module.new do + relations.each do |name| + define_method(name) do + relation_reader(name, relation_cache) + end end end end From 520bc19b293580b275ff875aff1e402962e221df Mon Sep 17 00:00:00 2001 From: Nikita Shilnikov Date: Tue, 7 Jan 2025 20:17:47 +0100 Subject: [PATCH 3/4] Revert "Remove leftovers from reverted commits" This reverts commit a75097236584f5e1edcc395c37dea6be5b442fba. --- repository/spec/spec_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/repository/spec/spec_helper.rb b/repository/spec/spec_helper.rb index 6ef8b7750..98673fda1 100644 --- a/repository/spec/spec_helper.rb +++ b/repository/spec/spec_helper.rb @@ -58,6 +58,8 @@ def self.remove_constants config.after do Test.remove_constants + ROM::Repository::RelationReader.relation_readers(nil) + ROM::Repository::RelationReader.relation_cache(Concurrent::Hash.new) end Dir[SPEC_ROOT.join('support/*.rb').to_s].each do |f| From 4c85e01f7b94a32085969a4a4165583f16e11a73 Mon Sep 17 00:00:00 2001 From: Nikita Shilnikov Date: Tue, 7 Jan 2025 20:36:23 +0100 Subject: [PATCH 4/4] Refactor optimized relation reader --- core/lib/rom/cache.rb | 2 +- core/lib/rom/container.rb | 10 ++ .../rom/plugins/relation/registry_reader.rb | 4 +- .../rom/setup/finalize/finalize_relations.rb | 18 ++-- repository/lib/rom/repository.rb | 3 +- .../lib/rom/repository/class_interface.rb | 8 +- .../lib/rom/repository/relation_reader.rb | 94 +++++++++---------- repository/lib/rom/repository/root.rb | 2 +- repository/spec/integration/plugin_spec.rb | 2 +- repository/spec/spec_helper.rb | 2 - 10 files changed, 77 insertions(+), 68 deletions(-) diff --git a/core/lib/rom/cache.rb b/core/lib/rom/cache.rb index d3c989b4c..cba0ae783 100644 --- a/core/lib/rom/cache.rb +++ b/core/lib/rom/cache.rb @@ -46,7 +46,7 @@ def inspect # @api private def initialize - @objects = Concurrent::Map.new + @objects = ::Concurrent::Map.new @namespaced = {} end diff --git a/core/lib/rom/container.rb b/core/lib/rom/container.rb index 70ebeb39f..91707d101 100644 --- a/core/lib/rom/container.rb +++ b/core/lib/rom/container.rb @@ -110,6 +110,16 @@ def self.new(gateways, relations, mappers, commands) end end + # @api private + attr_reader :cache + + # @api private + def initialize + super + + @cache = Cache.new + end + # Return registered gateways # # @return [HashGateway>] diff --git a/core/lib/rom/plugins/relation/registry_reader.rb b/core/lib/rom/plugins/relation/registry_reader.rb index fec206387..5a0602bca 100644 --- a/core/lib/rom/plugins/relation/registry_reader.rb +++ b/core/lib/rom/plugins/relation/registry_reader.rb @@ -14,9 +14,9 @@ class RegistryReader < ::Module EMPTY_REGISTRY = RelationRegistry.build(EMPTY_HASH).freeze # @api private - def initialize(klass:, relation_readers_module:) + def initialize(readers:) super() - klass.include relation_readers_module + include readers end # @api private diff --git a/core/lib/rom/setup/finalize/finalize_relations.rb b/core/lib/rom/setup/finalize/finalize_relations.rb index 056d4ab34..7b8565f64 100644 --- a/core/lib/rom/setup/finalize/finalize_relations.rb +++ b/core/lib/rom/setup/finalize/finalize_relations.rb @@ -9,14 +9,12 @@ class Finalize class FinalizeRelations attr_reader :notifications - module BuildRelationReaders - def self.build(relations) - Module.new do - relations.each do |name| - define_method(name) do - __registry__[name] - end - end + class RegistryReaders < ::Module + def initialize(relations) + super() + + relations.each do |name| + define_method(name) { __registry__[name] } end end end @@ -44,7 +42,7 @@ def initialize(gateways, relation_classes, notifications:, mappers: nil, plugins # rubocop:disable Metrics/AbcSize, Metrics/MethodLength def run! relation_registry = RelationRegistry.new do |registry, relations| - relation_readers_module = BuildRelationReaders.build(relation_names) + registry_readers = RegistryReaders.new(relation_names) @relation_classes.each do |klass| unless klass.adapter raise MissingAdapterIdentifierError, @@ -58,7 +56,7 @@ def run! "Relation with name #{key.inspect} registered more than once" end - klass.use(:registry_reader, klass: klass, relation_readers_module: relation_readers_module) + klass.use(:registry_reader, readers: registry_readers) notifications.trigger( 'configuration.relations.class.ready', diff --git a/repository/lib/rom/repository.rb b/repository/lib/rom/repository.rb index 857ddc362..f42e56f54 100644 --- a/repository/lib/rom/repository.rb +++ b/repository/lib/rom/repository.rb @@ -80,8 +80,9 @@ class Repository # @!method self.relation_reader # Get or set relation reader module - # @return [RelationReader] + # @return [Module] defines :relation_reader + relation_reader RelationReader struct_namespace ROM::Struct diff --git a/repository/lib/rom/repository/class_interface.rb b/repository/lib/rom/repository/class_interface.rb index 1bf549348..866d5e390 100644 --- a/repository/lib/rom/repository/class_interface.rb +++ b/repository/lib/rom/repository/class_interface.rb @@ -59,9 +59,11 @@ def [](name) def new(container = nil, **options) container ||= options.fetch(:container) - unless relation_reader - relation_reader(RelationReader.new(self, container.relations.elements.keys)) - include(relation_reader) + unless self < relation_reader + include relation_reader.new( + relations: container.relations.elements.keys, + cache: container.cache + ) end super(**options, container: container) diff --git a/repository/lib/rom/repository/relation_reader.rb b/repository/lib/rom/repository/relation_reader.rb index d365471ea..0b800f262 100644 --- a/repository/lib/rom/repository/relation_reader.rb +++ b/repository/lib/rom/repository/relation_reader.rb @@ -4,72 +4,72 @@ module ROM class Repository # @api private class RelationReader < ::Module - extend ::Dry::Core::ClassAttributes - - # @api private - attr_reader :klass - - # @api private - attr_reader :relations - - defines :relation_readers - - defines :mutex - mutex(Mutex.new) - - defines :relation_cache - relation_cache(Concurrent::Hash.new) - module InstanceMethods + private + # @api private - def set_relation(name) # rubocop:disable Naming/AccessorMethodName + def prepare_relation(name, **) container .relations[name] - .with(auto_struct: auto_struct, struct_namespace: struct_namespace) + .with( + auto_struct: auto_struct, + struct_namespace: struct_namespace + ) end - def relation_reader(name, relation_cache) - key = [name, auto_struct, struct_namespace] - relation_cache[key] ||= set_relation(name) + # @api private + def relation_reader(cache, ...) + cache_key = relation_cache_key(...) + cache.fetch_or_store(*cache_key) { prepare_relation(...) } end - end - # @api private - def mutex - ROM::Repository::RelationReader.mutex + # @api private + def relation_cache_key(name, **) + [name, auto_struct, struct_namespace] + end end # @api private - def initialize(klass, relations) - super() - @relations = relations - mutex.synchronize do - unless self.class.relation_readers - self.class.relation_readers( - build_relation_readers(relations, self.class.relation_cache) - ) + class Readers < ::Module + # @api private + attr_reader :cache + + def initialize(relations) + super() + + include InstanceMethods + + define_readers(relations) + end + + # @api private + def define_readers(relations) + cache = Cache.new + relations.each do |name| + define_readers_for_relation(cache, name) + end + end + + # @api private + def define_readers_for_relation(cache, name) + define_method(name) do |**kwargs| + relation_reader(cache, name, **kwargs) end end - klass.include self.class.relation_readers end # @api private - def included(klass) - super - klass.include(InstanceMethods) - end + def initialize(relations:, cache:) + super() - private + add_readers(relations, cache) + end # @api private - def build_relation_readers(relations, relation_cache) - Module.new do - relations.each do |name| - define_method(name) do - relation_reader(name, relation_cache) - end - end - end + def add_readers(relations, cache) + include cache.fetch_or_store(:relation_readers) { + Readers.new(relations) + } end end end diff --git a/repository/lib/rom/repository/root.rb b/repository/lib/rom/repository/root.rb index be0c891be..17d0a57dc 100644 --- a/repository/lib/rom/repository/root.rb +++ b/repository/lib/rom/repository/root.rb @@ -58,7 +58,7 @@ def self.inherited(klass) # @see Repository#initialize def initialize(*, **) super - @root = set_relation(self.class.root) + @root = prepare_relation(self.class.root) end end end diff --git a/repository/spec/integration/plugin_spec.rb b/repository/spec/integration/plugin_spec.rb index 641b129be..a500e5f24 100644 --- a/repository/spec/integration/plugin_spec.rb +++ b/repository/spec/integration/plugin_spec.rb @@ -12,7 +12,7 @@ def self.apply(target, **) target.prepend(self) end - def set_relation(*) + def prepare_relation(*) super.where { `1 = 0` } end end diff --git a/repository/spec/spec_helper.rb b/repository/spec/spec_helper.rb index 98673fda1..6ef8b7750 100644 --- a/repository/spec/spec_helper.rb +++ b/repository/spec/spec_helper.rb @@ -58,8 +58,6 @@ def self.remove_constants config.after do Test.remove_constants - ROM::Repository::RelationReader.relation_readers(nil) - ROM::Repository::RelationReader.relation_cache(Concurrent::Hash.new) end Dir[SPEC_ROOT.join('support/*.rb').to_s].each do |f|