[FIX] Serializers can now be defined before Rails initializes and cache store will be correctly set #1478

bf4 · 2016-01-28T21:59:27Z

Update: #1478 (comment) and #1478 (comment) #1478 (comment)

This is kind of a dumb fix for caching that demonstrates the
need in Rails for Serializer::_cache to be set after the controller is loaded
when Serializer::cache has already been called.

Notes on the issue

# Initialize app before any serializers are defined, for sanity's sake.
# Otherwise, you have to manually set perform caching.
#
# Details:
#
#1. Upon load, when AMS.config.perform_caching is true,
#    serializers inherit the cache store from ActiveModelSerializers.config.cache_store
#1. If the serializers are loaded before Rails is initialized (`Rails.application.initialize!`),
#    these values are nil, and are not applied to the already loaded serializers
#1. If Rails is initialized before any serializers are loaded, then the configs are set,
#    and are used when serializers are loaded
#1. In either case, `ActiveModelSerializers.config.cache_store`, and
#    `ActiveModelSerializers.config.perform_caching` can be set at any time before the serializers
#    are loaded,
#    e.g.  `ActiveModel::Serializer.config.cache_store ||=
#      ActiveSupport::Cache.lookup_store(ActionController::Base.cache_store ||
#      Rails.cache || :memory_store)`
#    and `ActiveModelSerializers.config.perform_caching = true`
#1. If the serializers are loaded before Rails is initialized, then,
#    you can set the `_cache` store directly on the serializers.
#    `ActiveModel::Serializer._cache ||=
#      ActiveSupport::Cache.lookup_store(ActionController::Base.cache_store ||
#      Rails.cache || :memory_store`
#    is sufficient.
#    Setting `_cache` to a truthy value will cause the CachedSerializer
#    to consider it cached, which will apply to all serializers (bug? :bug: )
#
# This happens, in part, because the cache store is set for a serializer
# when `cache` is called, and cache is usually called when the serializer is defined.
#
# So, there's now a 'workaround', something to debug, and a starting point.
Rails.application.initialize!

# HACK: Serializer::cache depends on the ActionController-dependent configs being set.
ActiveSupport.on_load(:action_controller) do
  require_relative 'fixtures'
end

bf4 · 2016-01-28T22:58:06Z

e.g. work done in https://github.com/rails-api/active_model_serializers/pull/1412/files#diff-8c03da6a006ecb5ab61b873ef8f5cba3R76

bf4 · 2016-01-31T02:27:00Z

Fixes #923

bf4 · 2016-02-05T05:07:22Z

lib/active_model/serializer/caching.rb

@@ -89,6 +89,10 @@ def fragmented(serializer)
        # https://github.com/rails-api/active_model_serializers/pull/1249#issuecomment-146567837
        def cache(options = {})
          self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching
+          serializer = self
+          ActiveSupport.on_load(:action_controller) do


duplicate since we can't rely on an app loading a controller

bf4 · 2016-02-15T06:05:13Z

lib/active_model/serializer/caching.rb

@@ -89,6 +89,10 @@ def fragmented(serializer)
        # https://github.com/rails-api/active_model_serializers/pull/1249#issuecomment-146567837
        def cache(options = {})
          self._cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching


The real problem is that calling this method should enable the cache via sentinel value, rather than setting the cache store dependent on an controller attribute. Each serializer could still have its own cache_store (though I'm not sure how often that would be used, and it causes extra complexity here).

Older versions of AMS used a method perform_caching named same as rails. I think that might be a good idea

easiest impl, have a class attribute perform_caching that acts as the sentinel. That was we don't have to change the api much... caching can still be turning on/off per serializer, but configured at load and disabled/enabled before use. (that's the impl that was in 0.8 #1568 )

groyoh · 2016-02-28T13:51:54Z

What about something like?

def _cache
  return @cache if @cache_set
  @cache_set = true
  @cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_caching
end
def _cache=(cache)
  @cache_set = true
  @cache = cache
end

AFAIK, the _cache method would never be called before the first resource is serialized. I think we can safely assume that at that point, the ActiveModelSerializers.config.cache_store and ActiveModelSerializers.config.perform_caching config should be already set. Thus, if we set the @cache at that point, we wouldn't need anykind of work-around. Adding _cache= then, we would still be able to set the cache manually if needed.

bf4 · 2016-02-28T20:58:50Z

Also a problem that _cache doubled as the serializer cache and a sentinel
to use caching
On Sun, Feb 28, 2016 at 7:51 AM Yohan Robert [email protected]
wrote:

What about something like?

def _cache
return @cache if @cache_set
@cache_set = true
@cache = ActiveModelSerializers.config.cache_store if ActiveModelSerializers.config.perform_cachingenddef _cache=(cache)
@cache_set = true
@cache = cacheend

AFAIK, the _cache method would never be called before the first resource
is serialized. I think we can safely assume that at that point, the
ActiveModelSerializers.config.cache_store and
ActiveModelSerializers.config.perform_caching config should be already
set. Thus, if we set the @cache at that point, we wouldn't need anykind
of work-around. Adding _cache= then, we would still be able to set the
cache manually if needed.

—
Reply to this email directly or view it on GitHub
#1478 (comment)
.

bf4 · 2016-03-13T08:11:40Z

ignore 4d6082f.. more objects and loses performance, oops

NullVoxPopuli · 2016-03-16T11:42:02Z

@bf4, could you rebase this and fix the rubocop warnings?

bf4 · 2016-03-16T18:15:27Z

I thought this was broken and I gave up for now

B mobile phone

On Mar 16, 2016, at 6:42 AM, L. Preston Sego III [email protected] wrote:

@bf4, could you rebase this and fix the rubocop warnings?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

NullVoxPopuli · 2016-03-16T18:16:46Z

Does the issue still need to be resolved?

bf4 · 2016-03-16T18:39:54Z

@NullVoxPopuli absolutely yes

bf4 · 2016-03-17T04:59:47Z

@NullVoxPopuli @remear @groyoh I believe I fixed it! There's definitely a bit of jankyness, so please comment if you see a better way.

A summary of our needs:

Each serializer can have its own cache store. _cache is a class attribute
We only want the serializer cache to be enabled when cache is called and perform_caching is true.

Summary of problems:

Right now, _cache serves as both the cache_store and the sentinel value that caching is enabled for the serializer.
If the serializer is loaded before Rails initializes or if the Rails initializes before AMS is required, the railtie won't fire, perform_caching will be falsy, and the cache store will never be set

Summary of solution:

See comments in diff. Basically, when cached is called, we now mark the serializer as 'cacheable', and instead of _cache, call cache_store to get the configure cache store. And when cache_store is called on a 'cacheable' serializer, it checks to see if the global cache store is set, and uses that. Phew

More details in PR description

bf4 · 2016-03-17T05:17:25Z

Benchmarking

Benchmarks confirm no performance hit. I also seem to be generating 3 less objects.

Before

Master HEAD db87f8d

bin/bench -r 5
caching on: caching serializers: gc off 634.3654607311329/ips; 1854 objects
caching off: caching serializers: gc off 614.575522426943/ips; 1854 objects
caching on: non-caching serializers: gc off 782.609603827895/ips; 1392 objects
caching off: non-caching serializers: gc off 706.3208987369165/ips; 1392 objects
caching on: caching serializers: gc off 626.183018453044/ips; 1854 objects
caching off: caching serializers: gc off 611.131393623337/ips; 1854 objects
caching on: non-caching serializers: gc off 800.8413437916081/ips; 1392 objects
caching off: non-caching serializers: gc off 730.1991428832154/ips; 1392 objects
caching on: caching serializers: gc off 635.9595295750097/ips; 1854 objects
caching off: caching serializers: gc off 613.6265970839457/ips; 1854 objects
caching on: non-caching serializers: gc off 793.4097406411736/ips; 1392 objects
caching off: non-caching serializers: gc off 727.5700645544579/ips; 1392 objects
caching on: caching serializers: gc off 645.498710493511/ips; 1854 objects
caching off: caching serializers: gc off 618.2892779001014/ips; 1854 objects
caching on: non-caching serializers: gc off 784.3447828298285/ips; 1392 objects
caching off: non-caching serializers: gc off 737.2795995214526/ips; 1392 objects
caching on: caching serializers: gc off 641.8325554022904/ips; 1854 objects
caching off: caching serializers: gc off 615.5090793528184/ips; 1854 objects
caching on: non-caching serializers: gc off 780.1478229085403/ips; 1392 objects
caching off: non-caching serializers: gc off 751.8133968350586/ips; 1392 objects
Benchmark results:
{
  "commit_hash": "db87f8d",
  "version": "0.10.0.rc4",
  "rails_version": "4.0.13",
  "benchmark_run[environment]": "2.2.3p173",
  "runs": [
    {
      "benchmark_type[category]": "caching on: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 645.499,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1854
    },
    {
      "benchmark_type[category]": "caching off: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 618.289,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1854
    },
    {
      "benchmark_type[category]": "caching on: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 800.841,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    },
    {
      "benchmark_type[category]": "caching off: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 751.813,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    }
  ]
}

After

PR HEAD a7eca81

bin/bench -r 5
caching on: caching serializers: gc off 588.0665690387696/ips; 1851 objects
caching off: caching serializers: gc off 571.1490068727436/ips; 1851 objects
caching on: non-caching serializers: gc off 754.977344046592/ips; 1392 objects
caching off: non-caching serializers: gc off 724.5287137156264/ips; 1392 objects
caching on: caching serializers: gc off 605.7295792722412/ips; 1851 objects
caching off: caching serializers: gc off 600.8992787976632/ips; 1851 objects
caching on: non-caching serializers: gc off 782.7515015587951/ips; 1392 objects
caching off: non-caching serializers: gc off 734.3344036062412/ips; 1392 objects
caching on: caching serializers: gc off 642.4116811888662/ips; 1851 objects
caching off: caching serializers: gc off 602.0263238031482/ips; 1851 objects
caching on: non-caching serializers: gc off 792.3897748109852/ips; 1392 objects
caching off: non-caching serializers: gc off 748.3909836691489/ips; 1392 objects
caching on: caching serializers: gc off 625.5546234938934/ips; 1851 objects
caching off: caching serializers: gc off 619.9850146085776/ips; 1851 objects
caching on: non-caching serializers: gc off 775.00739096189/ips; 1392 objects
caching off: non-caching serializers: gc off 751.4049208005396/ips; 1392 objects
caching on: caching serializers: gc off 635.8718384084705/ips; 1851 objects
caching off: caching serializers: gc off 612.727137954216/ips; 1851 objects
caching on: non-caching serializers: gc off 779.1109767055024/ips; 1392 objects
caching off: non-caching serializers: gc off 743.0832290755119/ips; 1392 objects
Benchmark results:
{
  "commit_hash": "a7eca81",
  "version": "0.10.0.rc4",
  "rails_version": "4.0.13",
  "benchmark_run[environment]": "2.2.3p173",
  "runs": [
    {
      "benchmark_type[category]": "caching on: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 642.412,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1851
    },
    {
      "benchmark_type[category]": "caching off: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 619.985,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1851
    },
    {
      "benchmark_type[category]": "caching on: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 792.39,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    },
    {
      "benchmark_type[category]": "caching off: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 751.405,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    }
  ]
}

NullVoxPopuli · 2016-03-17T11:27:41Z

lib/active_model/serializer/caching.rb

+        # We're using class variables here because it is a class attribute
+        # that is globally true for the `ActiveModel::Serializer` class; i.e. neither per subclass nor inherited.
+        #
+        # We're not using a class_attribute because of the special behavior in


thanks for the explanation! I like to read the thoughts behind decisions

NullVoxPopuli · 2016-03-17T11:29:33Z

lib/active_model/serializer/caching.rb

+        # This is to allow us to have a global config that can be set any time before
+        # `perform_caching` is called.
+        #
+        # One downside of this, is that subsequent setting of the global config will not change


just because the class var is memoized?

No, class variables are considered an anti-pattern in Ruby. This is distinct from class instance variables. Go and learn :)

NullVoxPopuli · 2016-03-17T11:39:36Z

Maybe something to look at, not sure if this'll work.

But, re: delegating to the config,

in active_model_serializers.rb#config, the Serializer class should autoload when the config method is called on, yeah?

in lib/ams/serializers.rb:

require 'active_model/serializer/collection_serializer'
require 'active_model/serializer/array_serializer'
require 'active_model/serializer/error_serializer'
require 'active_model/serializer/errors_serializer'
require 'active_model/serializer/include_tree'
require 'active_model/serializer/associations'
require 'active_model/serializer/attributes'
require 'active_model/serializer/caching'
require 'active_model/serializer/configuration'

what if configuration was at the top? would that help?

or is the loading race condition elsewhere?

bf4 · 2016-03-17T17:53:24Z

@NullVoxPopuli

in lib/ams/serializers.rb:
what if configuration was at the top? would that help?

nope. we're talking about when Rails is initialized vs. when the serializers are loaded vs. when the controller is first loaded. That's not a race condition. It's strictly determined by load order. More details in the PR description

groyoh · 2016-03-23T07:42:28Z

lib/active_model/serializer/caching.rb

+          end
+        end
+
+        def cache_enabled?


bf4 · 2016-03-25T02:25:45Z

Benchmark remains consistent with #1478 (comment):

bin/bench -r 5
caching on: caching serializers: gc off 636.6898235724902/ips; 1851 objects
caching off: caching serializers: gc off 583.3875333741602/ips; 1851 objects
caching on: non-caching serializers: gc off 740.5398081283838/ips; 1392 objects
caching off: non-caching serializers: gc off 720.725015737152/ips; 1392 objects
caching on: caching serializers: gc off 633.8812177805845/ips; 1851 objects
caching off: caching serializers: gc off 587.4968492563474/ips; 1851 objects
caching on: non-caching serializers: gc off 785.4658865833786/ips; 1392 objects
caching off: non-caching serializers: gc off 738.8961641332226/ips; 1392 objects
caching on: caching serializers: gc off 645.0143440480517/ips; 1851 objects
caching off: caching serializers: gc off 612.072503541359/ips; 1851 objects
caching on: non-caching serializers: gc off 792.3932952308679/ips; 1392 objects
caching off: non-caching serializers: gc off 748.0955311576615/ips; 1392 objects
caching on: caching serializers: gc off 633.5915591875073/ips; 1851 objects
caching off: caching serializers: gc off 606.7087955734825/ips; 1851 objects
caching on: non-caching serializers: gc off 797.7486292204281/ips; 1392 objects
caching off: non-caching serializers: gc off 752.7270149276842/ips; 1392 objects
caching on: caching serializers: gc off 649.7175402003957/ips; 1851 objects
caching off: caching serializers: gc off 605.4492794365647/ips; 1851 objects
caching on: non-caching serializers: gc off 788.2106786368261/ips; 1392 objects
caching off: non-caching serializers: gc off 746.5016796342492/ips; 1392 objects
Benchmark results:
{
  "commit_hash": "e38b362",
  "version": "0.10.0.rc4",
  "rails_version": "4.0.13",
  "benchmark_run[environment]": "2.2.3p173",
  "runs": [
    {
      "benchmark_type[category]": "caching on: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 649.718,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1851
    },
    {
      "benchmark_type[category]": "caching off: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 612.073,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1851
    },
    {
      "benchmark_type[category]": "caching on: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 797.749,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    },
    {
      "benchmark_type[category]": "caching off: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 752.727,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1392
    }
  ]
}

groyoh · 2016-03-25T10:20:29Z

Rakefile

@@ -43,7 +43,8 @@ Rake::TestTask.new do |t|
  t.libs << 'test'
  t.libs << 'lib'
  t.test_files = FileList['test/**/*_test.rb']
-  t.ruby_opts = ['-w -r./test/test_helper.rb']
+  t.ruby_opts = ['-r./test/test_helper.rb']
+  t.ruby_opts << ' -w' unless ENV['NO_WARN'] == 'true'


This should be already on master? Weird that it still shows in the diff.

groyoh · 2016-03-25T10:30:19Z

Looks good to me 🎉

bf4 added S: Bug V: 0.10.x S: Needs Team Discussion labels Jan 28, 2016

bf4 reviewed Feb 5, 2016
View reviewed changes

bf4 mentioned this pull request Feb 10, 2016

Cache not working #923

Open

bf4 reviewed Feb 15, 2016
View reviewed changes

bf4 mentioned this pull request Feb 22, 2016

Refactor fragment cache methods #1527

Closed

bf4 added the C: Performance: Caching label Mar 13, 2016

bf4 mentioned this pull request Mar 13, 2016

Caching doesn't improve performance #1586

Open

bf4 force-pushed the caching_fix branch 2 times, most recently from 090f24a to 4d6082f Compare March 13, 2016 08:07

bf4 force-pushed the caching_fix branch from 4d6082f to 6771d79 Compare March 13, 2016 08:18

bf4 force-pushed the caching_fix branch from 6771d79 to f0dc782 Compare March 17, 2016 04:52

bf4 force-pushed the caching_fix branch 3 times, most recently from bdd6cc4 to a7eca81 Compare March 17, 2016 05:10

bf4 changed the title ~~Trigger callback to set serializer#_cache when controller loaded~~ [FIX] Serializer's can be defined *before* Rails initializes and cache store will be correctly set. Mar 17, 2016

bf4 changed the title ~~[FIX] Serializer's can be defined *before* Rails initializes and cache store will be correctly set.~~ [FIX] Serializers can now be defined *before* Rails initializes and cache store will be correctly set. Mar 17, 2016

NullVoxPopuli reviewed Mar 17, 2016
View reviewed changes

groyoh reviewed Mar 23, 2016
View reviewed changes

lib/active_model/serializer/caching.rb

end

end

def cache_enabled?

Copy link

Member

groyoh Mar 23, 2016

Choose a reason for hiding this comment

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

👍

bf4 force-pushed the caching_fix branch from a7eca81 to a362520 Compare March 25, 2016 02:06

bf4 changed the title ~~[FIX] Serializers can now be defined *before* Rails initializes and cache store will be correctly set.~~ [FIX] Serializers can now be defined *before* Rails initializes and cache store will be correctly set Mar 25, 2016

bf4 added 6 commits March 24, 2016 22:20

Allow devs to opt out of test warnings

9ce3690

Add missing object context needed for tests to be run alone

27b514a

Trigger callback to set serializer#_cache when controller loaded

9953d7a

Separate enabling of caching and setting the cache store

c3c69a6

Add CHANGELOG [ci skip]

1230dd9

Simplify caching of value of config.perform_caching

dd60a37

bf4 force-pushed the caching_fix branch from 0774a7a to dd60a37 Compare March 25, 2016 03:22

bf4 mentioned this pull request Mar 25, 2016

Allow devs to opt out of test warnings #1619

Merged

groyoh reviewed Mar 25, 2016
View reviewed changes

groyoh merged commit 82da04d into rails-api:master Mar 25, 2016

bf4 deleted the caching_fix branch March 25, 2016 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] Serializers can now be defined before Rails initializes and cache store will be correctly set #1478

[FIX] Serializers can now be defined before Rails initializes and cache store will be correctly set #1478

bf4 commented Jan 28, 2016

bf4 commented Jan 28, 2016

bf4 commented Jan 31, 2016

bf4 Feb 5, 2016

bf4 Feb 15, 2016

bf4 Mar 9, 2016

bf4 Mar 9, 2016

groyoh commented Feb 28, 2016

bf4 commented Feb 28, 2016

bf4 commented Mar 13, 2016

NullVoxPopuli commented Mar 16, 2016

bf4 commented Mar 16, 2016

NullVoxPopuli commented Mar 16, 2016

bf4 commented Mar 16, 2016

bf4 commented Mar 17, 2016

bf4 commented Mar 17, 2016

NullVoxPopuli Mar 17, 2016

NullVoxPopuli Mar 17, 2016

bf4 Mar 17, 2016

NullVoxPopuli commented Mar 17, 2016

bf4 commented Mar 17, 2016

groyoh Mar 23, 2016

bf4 commented Mar 25, 2016

groyoh Mar 25, 2016

groyoh commented Mar 25, 2016

[FIX] Serializers can now be defined *before* Rails initializes and cache store will be correctly set #1478

[FIX] Serializers can now be defined *before* Rails initializes and cache store will be correctly set #1478

Conversation

bf4 commented Jan 28, 2016

Notes on the issue

bf4 commented Jan 28, 2016

bf4 commented Jan 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groyoh commented Feb 28, 2016

bf4 commented Feb 28, 2016

bf4 commented Mar 13, 2016

NullVoxPopuli commented Mar 16, 2016

bf4 commented Mar 16, 2016

NullVoxPopuli commented Mar 16, 2016

bf4 commented Mar 16, 2016

bf4 commented Mar 17, 2016

bf4 commented Mar 17, 2016

Benchmarking

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Mar 17, 2016

bf4 commented Mar 17, 2016

Choose a reason for hiding this comment

bf4 commented Mar 25, 2016

Choose a reason for hiding this comment

groyoh commented Mar 25, 2016

[FIX] Serializers can now be defined before Rails initializes and cache store will be correctly set #1478

[FIX] Serializers can now be defined before Rails initializes and cache store will be correctly set #1478