Improve Vertical Performance #22992
Improve Vertical Performance #22992
Conversation
davestgermain
force-pushed
the
dcs/perf-vertical
branch
3 times, most recently
from
f473524 to
9a32506
Compare
davestgermain
force-pushed
the
dcs/perf-vertical
branch
6 times, most recently
from
3bd7c2b to
a4ad703
Compare
|
jenkins run python |
davestgermain
changed the title
| Invalidate all keys tracked by the manager. | ||
| """ | ||
| for key in self.keys: | ||
| TieredCache.delete_all_tiers(key) |
There was a problem hiding this comment.
Could you please change this to use the request cache instead of the tiered cached? Setting things in memcached across processes doesn't buy that much performance, but it could cause weird bugs because memcached lives outside the transaction lifetime of the database. So for instance, if we do a save on a model within a transaction, and that transaction later gets rolled back, using the tiered cache like this would have already done a set in memcached with that model value. Then that bad/rolled-back value would be alive in memcached for another process that runs.
There was a problem hiding this comment.
Also, that way we don't have to worry about what happens when we serialize into memcache an older version of a model that just had a column added to it, etc.
There was a problem hiding this comment.
I made this invalidation class in order to cache the XBlockConfiguration queryset, which rarely changes (and only through the deployment process or django admin, right?). I don't think there's much win by storing it in the request, but it seems appropriate to keep it in memcached.
There was a problem hiding this comment.
Ah. I was more worried about the long term if this started being used for things like enrollments. I agree this is safe to do for something like config models.
But if that's the use case, then I think we were hitting at the same problem from different sides. :-(
There was a problem hiding this comment.
Ah, that will be a good change. When will it land? Can I help?
But it won't solve this particular problem -- the xblock config is accessed like XBlockConfiguration.objects.current_set().filter(deprecated=True), which would bypass your caching. We could add similar caching to current_set(), or I could keep this for now, with a big TODO to remove it once there's caching in config models?
There was a problem hiding this comment.
Yeah, it's close to merge, I just dropped it when we were getting close to the LabXchange launch and didn't pick it back up again. Let me do that today, as part of this story. I think I just need some minor fixes at this point.
I did think it addressed the XBlockConfiguration issue (at least most of them), because that was the major thing I was trying to eliminate when I was doing this optimization. But we should test it out in any event. Let me get that merged, then we can measure. I'll fix up that PR right now and tag you on review.
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Just a request to use the request cache instead of tiered cache.
davestgermain
force-pushed
the
dcs/perf-vertical
branch
2 times, most recently
from
8fb88ce to
3729fd4
Compare
davestgermain
force-pushed
the
dcs/perf-vertical
branch
2 times, most recently
from
f50ed25 to
c34706e
Compare
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
I have concerns about possible behavior changes with edxnotes. If there's an explanation for that or that part of it is taken out, I'm good to go with this PR. (If it stays, please include a comment in the PR about why the shift was made and why it's safe to do so.)
Thank you.
davestgermain
force-pushed
the
dcs/perf-vertical
branch
from
c34706e to
a5b0f71
Compare
|
jenkins run a11y |
|
Your PR has finished running tests. There were no failures. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
This adds some caching to reduce queries around rendering units/verticals (particularly when viewed from the new courseware MFE). On devstack, I measured between 10-20% improvement in response times.
Further improvements can be made around FBE/marketing injections. Currently, most of the time is spent outside of the xblock runtime.
TNL-7057