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

Cache transformed keys #402

Merged
merged 3 commits into from
Dec 29, 2024

Conversation

skryukov
Copy link
Contributor

@skryukov skryukov commented Dec 27, 2024

I noticed Alba was quite slow with the keys transformation feature enabled. After a quick review of the code, I decided to add a memoization mechanism directly in the main Alba module. This makes the code more transparent. While putting the memoization at the resource-level cache was also an option, it would still require checking Alba.inflector and Alba.symbolize_keys, which would look less obvious.

Additionally, I've updated the benchmark: it now supports Ruby 3.4, Rails and Representable serializers pass the output checks. I've updated the README (without the new results - I think it's better for @okuramasafumi to run them, especially interesting to see the latest json gem being faster than Oj.optimize_rails 😄). Finally, the script now displays the gem versions used at the end.

Benchmark results (we can drop transformation from benchmark once this PR is reviewed).

Note that alba_with_transformation is faster since the "pure" version calls regularize_key all the time.

BEFORE

ips:

Comparison:
               panko:      585.8 i/s
                alba:      186.5 i/s - 3.14x  slower
alba_with_transformation:       84.2 i/s - 6.96x  slower

memory:

Comparison:
               panko:     259178 allocated
                alba:    1002313 allocated - 3.87x more
alba_with_transformation:    2331053 allocated - 8.99x more

AFTER

ips:


Comparison:
               panko:      598.3 i/s
alba_with_transformation:      212.1 i/s - 2.82x  slower
                alba:      186.3 i/s - 3.21x  slower

memory:

Comparison:
               panko:     259178 allocated
alba_with_transformation:     826253 allocated - 3.19x more
                alba:    1002313 allocated - 3.87x more

@okuramasafumi
Copy link
Owner

Hi @skryukov, thank you for the patch! I didn't notice transforming keys is one of the bottlenecks. I highly appreciate it.
I think Alba.inflector rarely changes during runtime, so we might refactor it a bit:

diff --git a/lib/alba.rb b/lib/alba.rb
index afcf8de..e3626f2 100644
--- a/lib/alba.rb
+++ b/lib/alba.rb
@@ -117,6 +117,7 @@ module Alba
     #   When it's a Class or a Module, it should have some methods, see {Alba::DefaultInflector}
     def inflector=(inflector)
       @inflector = inflector_from(inflector)
+      reset_transform_keys
     end
 
     # @param block [Block] resource body
@@ -145,6 +146,7 @@ module Alba
     # Configure Alba to symbolize keys
     def symbolize_keys!
       @symbolize_keys = true
+      @_transformed_keys = nil
     end
 
     # Configure Alba to stringify (not symbolize) keys
@@ -171,21 +173,19 @@ module Alba
     def transform_key(key, transform_type:)
       raise Alba::Error, 'Inflector is nil. You must set inflector before transforming keys.' unless inflector
 
-      @_transformed_keys ||= Hash.new { |h, k| h[k] = Hash.new { |h2, k2| h2[k2] = Hash.new { |h3, k3| h3[k3] = {} } } }
-      @_transformed_keys[inflector][transform_type][@symbolize_keys][key] ||=
-        begin
-          key = key.to_s
-
-          k = case transform_type
-              when :camel then inflector.camelize(key)
-              when :lower_camel then inflector.camelize_lower(key)
-              when :dash then inflector.dasherize(key)
-              when :snake then inflector.underscore(key)
-              else raise Alba::Error, "Unknown transform type: #{transform_type}"
-              end
-
-          regularize_key(k)
-        end
+      @_transformed_keys[transform_type][key] ||= begin
+        key = key.to_s
+
+        k = case transform_type
+            when :camel then inflector.camelize(key)
+            when :lower_camel then inflector.camelize_lower(key)
+            when :dash then inflector.dasherize(key)
+            when :snake then inflector.underscore(key)
+            else raise Alba::Error, "Unknown transform type: #{transform_type}"
+            end
+
+        regularize_key(k)
+      end
     end
 
     # Register types, used for both builtin and custom types
@@ -213,7 +213,7 @@ module Alba
       @_on_error = :raise
       @_on_nil = nil
       @types = {}
-      @_transformed_keys = nil
+      reset_transform_keys
       register_default_types
     end
 
@@ -320,6 +320,10 @@ module Alba
         register_type(:"ArrayOf#{t}", check: ->(d) { d.is_a?(Array) && d.all? { _1.is_a?(t) } })
       end
     end
+
+    def reset_transform_keys
+      @_transformed_keys = Hash.new { |h, k| h[k] = {} }
+    end
   end
 
   reset!

This reduces nesting of a cache hash and instead resets the cache after some methods (Alba.inflector=, for example).
In my local environment (thank you for the benchmark improvement, too!), this refactoring makes it even faster than the original (maybe I'm wrong).
What do you think?

@okuramasafumi
Copy link
Owner

I noticed an interesting fact that with YJIT enabled, alba_with_transformation is slower than alba. Of course in general they are much faster than without YJIT, but I don't know how JIT changes the benchmark result in this situation.

@skryukov
Copy link
Contributor Author

skryukov commented Dec 28, 2024

Thanks for the review @okuramasafumi, I applied your patch (also called reset_transform_keys on both symbolize_keys! and stringify_keys!).

For me, running on Ruby 3.4.1 alba_with_transformation is still faster then alba 😅

YJIT + OJ:

Comparison:
               panko:      842.8 i/s
       turbostreamer:      429.9 i/s - same-ish: difference falls within error
         jserializer:      363.6 i/s - 2.32x  slower
alba_with_transformation:      347.6 i/s - 2.43x  slower
                alba:      300.4 i/s - 2.81x  slower
     fast_serializer:      244.7 i/s - 3.44x  slower
               rails:      168.6 i/s - 5.00x  slower
         blueprinter:      154.1 i/s - 5.47x  slower
       representable:      101.4 i/s - 8.31x  slower
          simple_ams:       94.5 i/s - 8.92x  slower
                 ams:       27.5 i/s - 30.70x  slower
         alba_inline:       14.5 i/s - 58.04x  slower

YJIT without Oj:

Comparison:
               panko:      857.3 i/s
       turbostreamer:      422.9 i/s - 2.03x  slower
alba_with_transformation:      413.0 i/s - 2.08x  slower
                alba:      379.8 i/s - 2.26x  slower
         jserializer:      370.2 i/s - 2.32x  slower
     fast_serializer:      244.4 i/s - 3.51x  slower
               rails:      161.3 i/s - 5.31x  slower
         blueprinter:      146.4 i/s - 5.86x  slower
       representable:      100.7 i/s - 8.52x  slower
          simple_ams:       87.9 i/s - 9.75x  slower
                 ams:       25.2 i/s - 33.96x  slower
         alba_inline:       14.4 i/s - 59.61x  slower

@okuramasafumi
Copy link
Owner

@skryukov Thank you, I cannot see your new commit on GitHub, maybe you didn't push it?
And for benchmark result, it's quite interesting. Alba might be faster than Oj? If so, I'd like to add this to README :)

@skryukov
Copy link
Contributor Author

Ooops, just pushed, sorry 😂

Regarding Oj: Yes, it now makes sense to compare JSON >= 2.9 with Oj. However, I believe the results will depend on the data. My colleague showed me some benchmarks where flat responses with simpler data types might still perform better when using Oj.

@okuramasafumi
Copy link
Owner

@skryukov Great, thank you!
Could you split your commits into two, benchmark-related and code-related ones? It's better because we can revert only code but keep benchmark improvements.

@skryukov skryukov force-pushed the cache-transformed-keys branch from 0d3906e to 82b54db Compare December 29, 2024 08:05
Copy link

codecov bot commented Dec 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.83%. Comparing base (f6c764c) to head (dc05334).
Report is 8 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #402   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files          14       14           
  Lines         592      599    +7     
  Branches      151      153    +2     
=======================================
+ Hits          591      598    +7     
  Misses          1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@okuramasafumi okuramasafumi merged commit 674bf15 into okuramasafumi:main Dec 29, 2024
68 checks passed
@okuramasafumi
Copy link
Owner

@skryukov Thank you!

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

Successfully merging this pull request may close these issues.

2 participants