V0 11 dev performance #1431

lgebhardt · 2024-01-15T16:44:35Z

Implements some caching of frequently computed values. Significantly reduces allocations and run times for larger requests.

All Submissions:

I've checked to ensure there aren't other open Pull Requests for the same update/change.
I've submitted a ticket for my issue if one did not already exist.
My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
I've used Github auto-closing keywords in the commit message or the description.
I've added/updated tests for this change.

New Feature Submissions:

I've submitted an issue that describes this feature, and received the go ahead from the maintainers.
My submission includes new tests.
My submission maintains compliance with JSON:API.

Bug fixes and Changes to Core Features:

I've included an explanation of what the changes do and why I'd like you to include them.
I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

Maintains compliance with JSON:API
Adequate test coverage exists to prevent regressions

lib/jsonapi/link_builder.rb

bf4 · 2024-01-15T21:00:08Z

lib/jsonapi/resource_common.rb

@@ -39,7 +39,7 @@ def _model
    end

    def id
-      _model.public_send(self.class._primary_key)
+      @id ||= _model.public_send(self.class._primary_key)


Off topic-- we'll want to handle composite primary keys here

That's an interesting point. I don't see anything in the spec to provide support for a multi attribute composite key in the JSONAPI section on identification. We could attempt to support this in JR by combining the composite key components, but I suspect it's going to run into the same issues I encountered trying to find a way to support an alternate public key field (one different from the primary key in the database). It was a harder problem than I initially thought it would be.

I figure someone could also just override this method. Since keys need to be consistently ordered, a custom synthetic key would be fine and keep the risk of a spec error off of jsonapi.

I think that's the way I would handle it too. To work with rails we would need an identity class to convert the composite string keys to an array and back. It would an interesting thing to test.

lib/jsonapi/resource_common.rb

bf4 · 2024-01-15T21:02:48Z

lib/jsonapi/resource_common.rb

@@ -1218,6 +1218,14 @@ def _clear_cached_attribute_options
        @_cached_attribute_options = {}
      end

+      def _clear_cached_resource_types_for_model
+        @_cached_resource_types_for_model = {}


No sure how allocations differ re garbage collection to set a new ivar vs calling clear

I'm not sure either. I assume, probably a bad thing to do, that a Set or Has would be cleaned up the same as the values it holds when no longer referenced. But I took a look at the code and reworked how the class variables for the caches are initialized and cleared. Probably doesn't matter much since this is code that's only run at startup. Still I hope it makes it a little clearer.

lib/jsonapi/resource_serializer.rb

lib/jsonapi/resource_tree.rb

bf4

Looks good

* Reduce number of string allocations in LinkBuilder * Consistently access `include_related` * Remove unused class variable * Cache `id` after retrieving it from the model * Cache `module_path` * Cache resource_klass_for and resource_type_for * Remove no longer used method _setup_relationship * Delete nil values without creating a new object * Rework resource naming for method caches

lgebhardt added 8 commits January 15, 2024 11:06

Reduce number of string allocations in LinkBuilder

867af02

Consistently access include_related

755142c

Remove unused class variable

3121ccb

Cache id after retrieving it from the model

a83920e

Cache module_path

f7c58cc

Cache resource_klass_for and resource_type_for

aad31ba

Remove no longer used method _setup_relationship

5cd779c

Delete nil values without creating a new object

c4eedf2

lgebhardt requested a review from bf4 January 15, 2024 17:47