Improve API controllers efficiency by selecting only columns needed for serialization by rhymes · Pull Request #5805 · forem/forem

rhymes · 2020-01-29T08:18:38Z

What type of PR is this? (check all applicable)

Description

The common thread here is avoiding SELECT * as much as possible. Especially with tables like users and articles that have respectively 150 and 90 columns:

PracticalDeveloper_development> select table_name, count(*) from information_schema.columns group by table_name having table_name in ('articles', 'users', 'organizations', 'podcast_episodes', 'podcasts', 'tags', 'tweets') order by count d
 esc;
+------------------+---------+
| table_name       | count   |
|------------------+---------|
| users            | 150     |
| articles         | 90      |
| organizations    | 42      |
| tweets           | 30      |
| podcast_episodes | 29      |
| tags             | 25      |
| podcasts         | 21      |
+------------------+---------+

thus I went through the code and did the following:

add .select(attributes) for all queries I could add it for
add some specs

This should have us some memory

rhymes · 2020-01-29T08:21:59Z


      private

+      INDEX_ATTRIBUTES_FOR_SERIALIZATION = %i[


you'll notice this pattern in the entire PR

This is something I really like about https://github.com/Netflix/fast_jsonapi, it forces you to be more explicit about the attributes that get serialized:

https://github.com/Netflix/fast_jsonapi#serializer-definition

We already have the gem installed, but only use it for a handful of serializers (see app/serializers/, they are all related to the Stackbit webhooks).

Anyway, your effort here seems to also be about reducing memory consumption when instantiating objects pre-serialization, not only about reducing payload size over the wire.

fast_jsonapi is in the back of my mind for API v1, as are JSON Schema and OpenAPI with things like https://github.com/interagent/committee

fast JSON has been something I have been dying to use since working with Elasticsearch requires so much serialization I think it could really speed things up when we hit higher volumes.

rhymes · 2020-01-29T08:23:13Z

-        existing_user_repos = Set.new(existing_user_repos)
+
+        existing_user_repos = current_user.github_repos.where(featured: true).
+          distinct.pluck(:github_id_code)


.distinct.pluck tells the DB to select only unique values, so we don't have to use Set.new or .to_set in memory

rhymes · 2020-01-29T08:23:52Z

        end

-        @repo = GithubRepo.find_or_create(fetched_repo_params(fetched_repo))
+        repo = GithubRepo.find_or_create(fetched_repo_params(fetched_repo))


no need to store it in an instance variable, as it's only used in this method

rhymes · 2020-01-29T08:24:32Z

+        client = create_octokit_client
+
+        fetched_repo = fetch_repo(client)


passed arguments over instance variables is a way of life :D

rhymes · 2020-01-29T08:28:41Z

+  # this is fully documented in the API docs
+  # see <https://github.com/thepracticaldev/dev.to/issues/4206> for more details
+  json.tag_list article.cached_tag_list_array
+  json.tags article.cached_tag_list


added this note because I had to keep these two attributes out of the partial, some of us are familiar with this discrepancy but it's still a good idea to document it

rhymes · 2020-01-29T08:28:51Z

+# /api/articles and /api/articles/:id have opposite representations
+# of `tag_list` and `tags and we can't align them without breaking the API,
+# this is fully documented in the API docs
+# see <https://github.com/thepracticaldev/dev.to/issues/4206> for more details
+json.tag_list @article.cached_tag_list
+json.tags @article.cached_tag_list_array


added this note because I had to keep these two attributes out of the partial, some of us are familiar with this discrepancy but it's still a good idea to document it

rhymes · 2020-01-29T08:29:11Z

 json.category             listing.category
 json.processed_html       listing.processed_html
 json.published            listing.published
-json.slug                 listing.slug


removed this as json.slug is already serialized at the top of the file

rhymes · 2020-01-29T08:29:27Z

@@ -0,0 +1,5 @@
+json.array! follows do |follow|


shared partial

rhymes · 2020-01-29T08:32:37Z

+        get :users
+        get :tags
+        get :organizations
+        get :podcasts


this have been changed into namespaces because we are using none of the CRUD operations created by resources. We don't need to generate all those routes

mstruve

This looks FANTASTIC! There is a lot here and I only was able to give it a quick pass as its time for me to make my way to the airport for Mohonk but once I give it another thorough look I think this is going to pay off big time.

mstruve · 2020-01-29T15:30:03Z

        error_not_found unless @chat_channel.has_member?(current_user)
      end
+
+      SHOW_ATTRIBUTES_FOR_SERIALIZATION = %i[id description channel_name].freeze


LOVE these constants!

mstruve · 2020-01-29T15:31:30Z

-        existing_user_repos = Set.new(existing_user_repos)
+
+        existing_user_repos = current_user.github_repos.where(featured: true).
+          distinct.pluck(:github_id_code)


mstruve · 2020-01-29T15:33:32Z


-      def organization_article?(reaction)
+      def org_article?(reaction)
        reaction.reactable_type == "Article" && reaction.reactable.organization_id


Should we update this to check for the actual organization rather than just the id in the event that the organization is removed?

Don't know if we want to do it in the same PR but yes!

Holding you too that so I dont see it in HB in the future 😉

mstruve · 2020-01-29T15:54:02Z

-json.published_at             @article.published_at&.utc&.iso8601
-json.last_comment_at          @article.last_comment_at&.utc&.iso8601
-json.published_timestamp      @article.published_timestamp
+json.partial! "article", article: @article


Super awesome refactor with definitely save us from getting out of sync at some point

mstruve · 2020-01-29T15:56:42Z

-      expect(json_response.first["slug"]).to eq(ClassifiedListing.last.slug)
-      expect(json_response.first["user"]).to include("username")
-      expect(json_response.first["user"]["username"]).not_to be_empty
+      expect(response.parsed_body.size).to eq(7)


parsed_body is seriously one of my new favorite shortcuts!

ahah learned it from you :D

citizen428 · 2020-01-30T03:49:27Z


      private

+      INDEX_ATTRIBUTES_FOR_SERIALIZATION = %i[


This is something I really like about https://github.com/Netflix/fast_jsonapi, it forces you to be more explicit about the attributes that get serialized:

https://github.com/Netflix/fast_jsonapi#serializer-definition

We already have the gem installed, but only use it for a handful of serializers (see app/serializers/, they are all related to the Stackbit webhooks).

Anyway, your effort here seems to also be about reducing memory consumption when instantiating objects pre-serialization, not only about reducing payload size over the wire.

citizen428 · 2020-01-31T03:35:05Z

+        positive_reactions_count created_at edited_at last_comment_at published
+        updated_at
+      ].freeze
+      private_constant :INDEX_ATTRIBUTES_FOR_SERIALIZATION


Despite the singular name, private_constant does take a list of constants to make private. Maybe this makes the repetition a bit more bearable.

private_constant :INDEX_ATTRIBUTES_FOR_SERIALIZATION, :SHOW_ATTRIBUTES_FOR_SERIALIZATION, :ME_ATTRIBUTES_FOR_SERIALIZATION

citizen428

I think this is an awesome PR, thanks for this @rhymes 🎉

In general — and we talked about this before — I think we do a bit too much AR directly in the controller, whereas in the past I tended towards query objects as outlined in this post: https://codeclimate.com/blog/7-ways-to-decompose-fat-activerecord-models/

But that's a wider discussion since it introduces a whole new pattern.

mstruve

LGTM 🚀

mstruve · 2020-01-31T20:16:03Z


      private

+      INDEX_ATTRIBUTES_FOR_SERIALIZATION = %i[


fast JSON has been something I have been dying to use since working with Elasticsearch requires so much serialization I think it could really speed things up when we hit higher volumes.

mstruve · 2020-01-31T20:29:02Z


-      def organization_article?(reaction)
+      def org_article?(reaction)
        reaction.reactable_type == "Article" && reaction.reactable.organization_id


Holding you too that so I dont see it in HB in the future 😉

rhymes · 2020-02-01T14:13:34Z

fast JSON has been something I have been dying to use since working with Elasticsearch requires so much serialization I think it could really speed things up when we hit higher volumes.

One thing we could already do is to use Oj as a serializer (which we're using for "fast json" already) for ActiveSupport::JSON::Encoding. I have this enabled in another project who serializes A LOT and it was very helpful when we switched. I haven't proposed this yet because I have no idea if our API endpoints are fast or slow TBH

In that project I have a config/initializers/json_encoding.rb like this:

# see https://precompile.com/2015/07/25/rails-activesupport-json.html
module ActiveSupport
  module JSON
    module Encoding
      # Use Oj as JSON encoder
      class OjEncoder < JSONGemEncoder
        def encode(value)
          ::Oj.dump(value.as_json, mode: :compat)
        end
      end
    end
  end
end

ActiveSupport.json_encoder = ActiveSupport::JSON::Encoding::OjEncoder

Holding you too that so I dont see it in HB in the future

as soon as we merge this I'll send a PR for that

rhymes added 14 commits January 28, 2020 13:31

Improve ArticlesController

d566ad1

Improve ChatChannelsController

73b29c9

Improve ClassifiedListingsController

af2bb7c

Improve FollowersController

8987e46

Improve FollowingsController

c5019c5

Improve FollowsController

818e8f1

Improve GithubReposController

63c2c45

Improve ReactionsController

b2d4e7a

Improve TagsController

472a662

Improve UsersController

aefc9fc

Improve WebhooksController

fd54de1

Improve UsersController

f473874

Fix chat channels spec

28ffc3d

Improve ArticleApiIndexService

3ff68e9

rhymes requested a review from a team January 29, 2020 08:18

pr-triage Bot added the PR: unreviewed bot applied label for PR's with no review label Jan 29, 2020

rhymes removed the request for review from a team January 29, 2020 08:18

rhymes commented Jan 29, 2020

View reviewed changes

mstruve reviewed Jan 29, 2020

View reviewed changes

Remove commented line

96f836a

citizen428 reviewed Jan 30, 2020

View reviewed changes

rhymes added 3 commits January 30, 2020 11:37

Keep attributes to select out of ArticleApiIndexService

ba55d40

Splat constants

e47a6da

Make those constants private for real

1ac53ef

rhymes requested review from citizen428 and mstruve January 30, 2020 10:48

citizen428 reviewed Jan 31, 2020

View reviewed changes

citizen428 approved these changes Jan 31, 2020

View reviewed changes

pr-triage Bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 31, 2020

mstruve approved these changes Jan 31, 2020

View reviewed changes

weekly-digest Bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

mstruve merged commit fd5643b into forem:master Feb 6, 2020

pr-triage Bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 6, 2020

rhymes deleted the rhymes/improve-api-controllers-efficiency branch February 6, 2020 12:38

This was referenced Feb 6, 2020

Fix serialization error with video_thumbnail_url #5933

Merged

Refactor API Reactions controller and add specs #5936

Merged

weekly-digest Bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #5980

Closed

		client = create_octokit_client

		fetched_repo = fetch_repo(client)

Conversation

rhymes commented Jan 29, 2020

What type of PR is this? (check all applicable)

Description

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstruve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

citizen428 Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

citizen428 left a comment

Choose a reason for hiding this comment

Uh oh!

mstruve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhymes commented Feb 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

citizen428 Jan 31, 2020 •

edited

Loading