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

improve performance for n+1s #817

Merged
merged 3 commits into from May 2, 2017
Merged

improve performance for n+1s #817

merged 3 commits into from May 2, 2017

Conversation

@BenMorganIO
Copy link
Collaborator

BenMorganIO commented Apr 4, 2017

This guy solves some n+1 problems in the application. The solution is global. I need to take time though to scope the n+1s down to just what attribute constants we're actually using.

Tried using COLLECTION_ATTRIBUTES but that didn't work out very well. Didn't remove a single n+1. As per n+1 issues, I think sticking to ATTRIBUTE_TYPES is a good first step. I think we should integrate this with Administate::Page::Collection.


dashboard.class::ATTRIBUTE_TYPES.map do |key, value|
key if association_classes.include?(value) ||
association_classes.include?(value.try :deferred_class)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 4, 2017

Use 4 (not 7) spaces for indenting a condition in an if statement spanning multiple lines.
Add parentheses to nested method call value.try :deferred_class.

This comment has been minimized.

Copy link
@nickcharlton

nickcharlton Apr 29, 2017

Member

This feels a little messy. Maybe we can split the checking here into one line, the try to another?

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO Apr 29, 2017

Author Collaborator

Yeah, I think was thinking of doing that. I'm going to take another stab at this.

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base
def index
search_term = params[:search].to_s.strip
resources = Administrate::Search.new(resource_resolver, search_term).run
resources = resources.includes(*resource_includes) if resource_includes.any?

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 4, 2017

Line is too long. [82/80]

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO Apr 25, 2017

Author Collaborator

Hmm this really looks like it needs to be moved to the Administrate::Page::Collection model.

This comment has been minimized.

Copy link
@nickcharlton

nickcharlton Apr 29, 2017

Member

Let's try that and see how it goes?

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO Apr 29, 2017

Author Collaborator

Well, I thought about moving it to the collection class, but that didn't work out the way I thought it would. Instead, it's been moved to the dashboard and we pass the includes from there.

@kevinelliott

This comment has been minimized.

Copy link

kevinelliott commented Apr 24, 2017

I would love to see this make it in.

@BenMorganIO BenMorganIO force-pushed the BenMorganIO:n+1s branch from 74b076b to 645fd60 Apr 28, 2017
@nickcharlton nickcharlton added this to the v0.6.0 milestone Apr 29, 2017
@BenMorganIO BenMorganIO force-pushed the BenMorganIO:n+1s branch from 645fd60 to 23eda32 Apr 29, 2017
BenMorganIO added 2 commits Apr 29, 2017
@BenMorganIO BenMorganIO force-pushed the BenMorganIO:n+1s branch 2 times, most recently from e8bb8ab to 53f8bca Apr 29, 2017
@nickcharlton nickcharlton merged commit 1ce0a5a into thoughtbot:master May 2, 2017
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!
@BenMorganIO BenMorganIO deleted the BenMorganIO:n+1s branch May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.