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 read_multi #1372

Merged
merged 1 commit into from
Feb 11, 2016
Merged

support read_multi #1372

merged 1 commit into from
Feb 11, 2016

Conversation

LcpMarvel
Copy link
Contributor

No description provided.

@@ -116,7 +116,7 @@ def self.fragmented(serializer)
# @todo require less code comments. See
# https://github.com/rails-api/active_model_serializers/pull/1249#issuecomment-146567837
def self.cache(options = {})
self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching
self._cache = ActionController::Base.cache_store if ActiveModelSerializers.config.perform_caching
Copy link
Member

Choose a reason for hiding this comment

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

Any change to AMS.config.cache_store should be where it is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the ActiveModelSerializers.config.cache_store is always nil in my project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i do extra config for ActiveModelSerializers.config.cache_store? @bf4

Copy link
Member

Choose a reason for hiding this comment

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

What did yoi have in mind?

B mobile phone

On Dec 13, 2015, at 7:56 PM, Michael Lyu [email protected] wrote:

In lib/active_model/serializer.rb:

@@ -116,7 +116,7 @@ def self.fragmented(serializer)
# @todo require less code comments. See
# #1249 (comment)
def self.cache(options = {})

  •  self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching
    
  •  self._cache = ActionController::Base.cache_store if ActiveModelSerializers.config.perform_caching
    
    Should i do extra config for ActiveModelSerializers.config.cache_store? @bf4


Reply to this email directly or view it on GitHub.

@bf4
Copy link
Member

bf4 commented Dec 11, 2015

@LcpMarvel Thanks for this 💯

I'll review. It will need some tests and documentation. Can you add that?

ref: #827

end
end

cache_keys.reject(&:nil?)
Copy link
Member

Choose a reason for hiding this comment

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

vs. compact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

require 'benchmark'

n = 100000

Benchmark.bm do |x|
  x.report do
    n.times do
      Array.new(100) { nil }.reject(&:nil?)
    end
  end

  x.report do
    n.times do
      Array.new(100) { nil }.compact
    end
  end
end
       user     system      total        real
   0.970000   0.020000   0.990000 (  0.984381)
   0.630000   0.010000   0.640000 (  0.633962)

@LcpMarvel LcpMarvel force-pushed the support_read_multi branch 2 times, most recently from bcb12a2 to 66b67f5 Compare December 14, 2015 03:18
@LcpMarvel
Copy link
Contributor Author

initializer 'active_model_serializers.caching' do
  ActiveSupport.on_load(:action_controller) do
    binding.pry

    ActiveModelSerializers.config.cache_store     = ActionController::Base.cache_store
    ActiveModelSerializers.config.perform_caching = Rails.configuration.action_controller.perform_caching
  end
end

I found that it does not work for Rails 4.2.5.

ActionController::Base.cache_store # => nil
Rails.configuration.cache_store # => :dalli_store
Rails.configuration.action_controller.perform_caching # => true

My config is

Rails.application.configure do
  # Settings specified here will take precedence over those in config/application.rb.

  # In the development environment your application's code is reloaded on
  # every request. This slows down response time but is perfect for development
  # since you don't have to restart the web server when you make code changes.
  config.cache_classes = false

  # Do not eager load code on boot.
  config.eager_load = false

  # Show full error reports and disable caching.
  config.consider_all_requests_local       = true
  config.action_controller.perform_caching = true
  config.cache_store = :dalli_store

  # Don't care if the mailer can't send.
  config.action_mailer.raise_delivery_errors = false

  # Print deprecation notices to the Rails logger.
  config.active_support.deprecation = :log

  # Raise an error on page load if there are pending migrations.
  config.active_record.migration_error = :page_load

  # Debug mode disables concatenation and preprocessing of assets.
  # This option may cause significant delays in view rendering with a large
  # number of complex assets.
  config.assets.debug = true

  # Asset digests allow you to set far-future HTTP expiration dates on all assets,
  # yet still be able to expire them through the digest params.
  config.assets.digest = true

  # Adds additional error checking when serving assets at runtime.
  # Checks for improperly declared sprockets dependencies.
  # Raises helpful error messages.
  config.assets.raise_runtime_errors = true
end

@bf4
Copy link
Member

bf4 commented Dec 14, 2015

I wonder why. That's a strange default

B mobile phone

On Dec 13, 2015, at 10:19 PM, Michael Lyu [email protected] wrote:

initializer 'active_model_serializers.caching' do
  ActiveSupport.on_load(:action_controller) do
    binding.pry

    ActiveModelSerializers.config.cache_store     = ActionController::Base.cache_store
    ActiveModelSerializers.config.perform_caching = Rails.configuration.action_controller.perform_caching
  end
end

I found that it does not work for Rails 4.2.5.
The ActionController::Base.cache_store is nil, but theRails.configuration.action_controller.perform_caching is true.


Reply to this email directly or view it on GitHub.

@LcpMarvel
Copy link
Contributor Author

I think we should use ActiveModelSerializers.config.cache_store = ActiveSupport::Cache.lookup_store(Rails.configuration.cache_store) to fix this problem.

@bf4
Copy link
Member

bf4 commented Dec 14, 2015

@LcpMarvel Thanks for looking into this. I'm learning more about this part of the Rails stack.

It looks to me like we are safest getting the 'Rails' cache store via ActiveSupport::Cache.lookup_store(ActionController::Base.cache_store || Rails.cache) since that won't blow up in the absence of a Rails app (i.e. Rails.configuration.cache_store won't raise undefined method 'config' on nil). Otherwise, we can append the condition Rails.application && Rails.configuration.cache_store (i.e. Rails.application.config.cache_store). Then, the default for nil cache_store would be the Memory cache.

I also noticed in the ActionController::Cache code that there's a caching_enabled? check that combines perform_caching with the presence of config.cache_store. I like that idea. In that case, we probably wouldn't want to default to the memory store when no store is set.

Any reason you're using read_multi over fetch_multi? fetch_multi calls read_multi.

@LcpMarvel
Copy link
Contributor Author

Yes, here we have no necessary for calling block because of the cache_check method.

@LcpMarvel
Copy link
Contributor Author

👍

@LcpMarvel
Copy link
Contributor Author

I wonder know how to set cache key by current_user.id because i have to handle the response by user.

My solution is

def attributes(requested_attrs = nil)
  attrs = super

  self.class.cache key: "#{scope.id}/#{scope.updated_at.to_i}"
  # business logic

  attrs
end

Can you give some suggestions?

@bf4
Copy link
Member

bf4 commented Dec 14, 2015

@LcpMarvel

I wonder know how to set cache key by current_user.id because i have to handle the response by user.

see https://github.com/rails-api/active_model_serializers/pull/1371/files#diff-683b15b4d7c82f436aaeada1733d9eecR97 https://github.com/bf4/active_model_serializers/blob/1b2324bd021b7e9adc5be11c123dd90286f3541b/docs/general/serializers.md#cache_key

# Uses a custom non-time-based cache key
def cache_key
  "#{self.class.name.downcase}/#{self.id}"
end

Do you use scope? If so, I could use some help documenting it #1252

@LcpMarvel
Copy link
Contributor Author

Yes, I think we can use any variables from the instance_options.

@@ -24,6 +24,15 @@ def fragment_cache(cached_hash, non_cached_hash)
private

def serializable_hash_for_collection(options)
if options[:batch_cache].blank? && ActiveModelSerializers.config.cache_store.present?
Copy link
Member

Choose a reason for hiding this comment

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

batch_cache is a purely internal serialization option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bf4 I think it's free to users. haha.
Users can pass in batch_cache: { disable: true } for preventing batch_cache.

@LcpMarvel LcpMarvel force-pushed the support_read_multi branch 3 times, most recently from eabafb2 to ab1b06c Compare December 16, 2015 05:08
@LcpMarvel LcpMarvel force-pushed the support_read_multi branch 3 times, most recently from 7041680 to 1037c8e Compare December 16, 2015 06:02
@LcpMarvel
Copy link
Contributor Author

@bf4 I have refactored

# Read cache from cache_store, and set those into options[:batch_cache]
# @return [Hash] a batch of cache
def batch_cache(options)
return {} if options[:batch_cache].present? || ActiveModelSerializers.config.cache_store.blank?
Copy link
Member

Choose a reason for hiding this comment

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

simpler to follow if the logic is something like think below?

def serializable_hash_for_collection(options)
+   cache_attributes(options)
  serializer.map { |s| Attributes.new(s, instance_options).serializable_hash(options) }
end

and

def cache_attributes(options)
  options[:cached_attributes] ||=
    begin
      if ActiveModelSerializers.config.cache_store.nil?
        {}
      else
        keys = CachedSerializer.object_cache_keys(serializer, @include_tree)
        if keys.any?
          ActiveModelSerializers.config.cache_store.read_multi(*keys)
        else
          {}
        end
      end
    end
end

def cached_attributes(cache_key)
  options[:cached_attributes].fetch(cache_key) do
    block_given? ? yield : raise(KeyError, "No key #{cache_key}. Keys are #{options[:cached_attributes].keys}")
  end
end

then

         def resource_object_for(options)
 -          cache_check(serializer) do
+          cached_serializer = CachedSerializer.new(serializer)
+          cached_attributes(cached_serializer.cache_key) do
+            cached_serializer.cache_check(self) do
                serializer.attributes(options[:fields])
              end
            end
          end

@LcpMarvel LcpMarvel force-pushed the support_read_multi branch 4 times, most recently from 6f437b2 to a04a00a Compare January 5, 2016 12:54
@bf4 bf4 merged commit 43312fa into rails-api:master Feb 11, 2016
bf4 added a commit that referenced this pull request Feb 11, 2016
@bf4
Copy link
Member

bf4 commented Feb 11, 2016

@LcpMarvel Thanks for this. I don't recall why we didn't merge it. I added a changelog entry in b45f7b4

cached_attributes(cached_serializer) do
cached_serializer.cache_check(self) do
serializer.attributes(options[:fields])
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't recall why we removed the cache_check method here

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

Successfully merging this pull request may close these issues.

2 participants