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

Home page: Optimize query for authorized students #2652

Merged
merged 2 commits into from Oct 1, 2019

Conversation

@kevinrobinson
Copy link
Contributor

kevinrobinson commented Oct 1, 2019

Who is this PR for?

all educators

What does this PR do?

This optimizes authorized { Student.active } for educators with section assignments, impacting the home page feed, my students page, and many other parts of the product.

It does this by removing calls to Educator#labels that end up doing authorization checks to see if the educator has any sections, in the process of computing dynamic labels. Since the only labels needs in the authorization method itself are static, optimize the call there to only look at static labels.

I also initially reordered the condition checks related to housemaster authorization; these would run even if the feature was disabled through an env flag. This actually led to consistently slower p95 values on the PerfTest, so I noted it and backed that out.

Data from Somerville, for sample of 5% of educators:

> PerfTest.authorized(0.05);nil
before: all_for_educator          median: 911ms   p95: 7290ms
after:  all_for_educator          median: 423ms   p95: 993ms


> PerfTest.feed(0.05);nil
# before:    p50: 739    p95: 3939
# after:     p50: 713    p95: 1486

New Bedford will soon be impacted by this change, but for now the speedups only impact Somerville folks.

Separately, after this work and measurement, I noticed the PerfTest class defaults to all educators, and so updated that to use Educator.active going forward.

Checklists

Which features or pages does this PR touch?

  • Home page
  • My Students
  • Core

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Manual testing made more sense here
@kevinrobinson

This comment has been minimized.

Copy link
Contributor Author

kevinrobinson commented Oct 1, 2019

selfie

@kevinrobinson kevinrobinson merged commit 97c440a into master Oct 1, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kevinrobinson kevinrobinson deleted the perf/authorized-students branch Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.