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

Slightly sketchy speed-up for /rotation/ #6773

Merged
merged 2 commits into from Nov 12, 2019
Merged

Conversation

tsutton
Copy link
Contributor

@tsutton tsutton commented Nov 9, 2019

Context: #5471.

Changes: call Flask's builtin url_for once to retrieve a template url and then just use string formatting to build the url for each card.

@tsutton tsutton added the do not merge Do not merge to master and deploy this PR when the build passes label Nov 9, 2019
@TravisBuddy
Copy link

Hey @tsutton,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: c7b83080-02b3-11ea-bbb5-2bfb3cbbfe32

@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #6773 into master will increase coverage by 0.02%.
The diff coverage is 85%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6773      +/-   ##
=========================================
+ Coverage   41.88%   41.9%   +0.02%     
=========================================
  Files         257     257              
  Lines       13314   13329      +15     
  Branches     2020    2023       +3     
=========================================
+ Hits         5577    5586       +9     
- Misses       7491    7492       +1     
- Partials      246     251       +5
Impacted Files Coverage Δ
decksite/view.py 63.27% <85%> (-0.19%) ⬇️
logsite/api.py 42.85% <0%> (ø) ⬆️
maintenance/elo.py 0% <0%> (ø) ⬆️
magic/multiverse.py 5.45% <0%> (ø) ⬆️
decksite/views/person_achievements.py 35% <0%> (ø) ⬆️
decksite/data/rule.py 23.63% <0%> (ø) ⬆️
logsite/data/match.py 75% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30f5bd4...bc4dbb4. Read the comment docs.

Copy link
Member

@bakert bakert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the total speedup late in full rotation week? About 1s?

What do you think about moving this to shared_web.base_view.BaseView.url_for or (probably more appropriate) decksite.view.View.url_for and doing a little bit of magic based on the input params. Then we could change literally every url_for to use the new function and do this for anything we see more than a few dozen times in a request?

@bakert
Copy link
Member

bakert commented Nov 9, 2019

We could even have it store counts of (template + '|' + len(args)) occurrences and flag anything over 50 or something.

@silasary silasary added beta test PR-only. Do not merge to master, instead deploy it to test site. and removed do not merge Do not merge to master and deploy this PR when the build passes labels Nov 10, 2019
@silasary silasary temporarily deployed to penny-dreadful-test November 10, 2019 02:10 Inactive
@silasary silasary removed the beta test PR-only. Do not merge to master, instead deploy it to test site. label Nov 12, 2019
@vorpal-buildbot vorpal-buildbot merged commit 62ad4ad into master Nov 12, 2019
@vorpal-buildbot vorpal-buildbot added Pending-on-LOGS Used by the build bot Pending-on-PROD Used by the build bot Seen-on-LOGS and removed Pending-on-LOGS Used by the build bot labels Nov 12, 2019
@vorpal-buildbot
Copy link
Contributor

Seen on LOGS (created by @tsutton and merged by @vorpal-buildbot 47 seconds ago) Please check your changes!

@vorpal-buildbot vorpal-buildbot added Seen-on-PROD and removed Pending-on-PROD Used by the build bot labels Nov 12, 2019
@vorpal-buildbot
Copy link
Contributor

Seen on PROD (created by @tsutton and merged by @vorpal-buildbot 1 minute and 1 second ago) Please check your changes!

@silasary silasary deleted the speed-rotation-page branch June 9, 2020 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants