[lainets]

Description

What?

• Refactors CachedContent/Points into separate objects: each module and exercise is stored separately. Allows loading points for individual exercises or loading points for the instance without loading each exercise and module points as well.
• Adds type hints to the cache entries.
• Replaces UserExerciseSummary with the cached SubmittableExerciseEntry.
• Transaction support for cache operations: cache changes are only committed if the transaction is.
• Support for loading multiple cache entries at once

BEHAVIOUR CHANGES:

• course front page now shows "Not submitted" on confirm_the_level exercises without submissions or confirmable points
  • matches the exercise pages
  • earlier it showed nothing
• points_badge submission status:
  • shows submission status as long as it hasn't been graded
  • earlier submission object wouldn't show status if confirm_the_level and status == Submission.STATUS.WAITING (in which case it showed "Submitted")
    • i.e. "In grading" vs "Submitted" for submissions that have confirm_the_level and status == Submission.STATUS.WAITING
• exercise page points badge now links to the best unofficial submission instead of first when all submissions are unofficial
  • seemed to be a bug in UserExerciseSummary
• exercise is 'passed' in cache only if best submission is READY but UserExerciseSummary checked that it is not UNOFFICIAL
  • ResultTable also only checks READY submissions when calculating the grade for an exercise
• missing_points defaulted to False when unrevealed for summary but True for cached (if points_to_pass > 0)
  • doesnt matter as the missing_points value isnt used when unrevealed
• a group is 'fixed' if there are any submissions to an exercise
  • previously only non-error submissions counted
• exercise boxes do not show unofficial submissions as part of the out of max submissions count
• course front page no longer shows unofficial points for exercises if only unofficial submissions exist
  • this is how it worked for exercise pages
• course front page only shows points for visible exercises
• the points of non-submittable exercises in the cache now depent on all descendants and not only the immediate children
• e.g. a chapter's points used to only include the points of submittable exercises that were direct children. Now they also include the points of submittable exercises in children's children and their children etc.

Why?

• Points calculation code was duplicated in UserExerciseSummary (and still is elsewhere, see Refactoring points calculation code #888).
• There were differences in the points shown in UI on the front page and the exercise pages.
• CachedContent (one for each instance) was duplicated in each CachedPoints (one for each user for each instance)
• It wasn't very clear what is contained in the cache as there were no type hints, and the generation code was a bit confusing
• CachedContent and CachedPoints sometimes grew too large to be saved in the cache.
• Transaction support:
  • this was originally required for tests to pass but might not actually be needed anymore with the dependency checks
  • the cache isn't invalidated unneccessarily if the transaction is rolled back
  • newly generated cache entries might be based on invalid data and aren't saved if the transaction is rolled back
  • write to cache happens all at once at the end of the transaction: more performant
  • invalidation only happens at the end of the transaction which is were the changes actually happen to the database
    • it was possible before that the cache was invalidated due to database changes, then generated (by a different request) and the database changes were committed afterwards, leaving the cache in an invalid state.

How?

Introduces a new cache base class:

• Enables type hints for cache entries
• Each type hinted attribute is saved to cache (except if they are marked with NoCache, ClassVar or InitVar)
• Supports saving objects in the inheritance tree separately, allowing inheriting from other cache objects without duplicating the contents in the cache.
• Supports dependencies between cache entries, allowing an entry to be considered invalid if any of the dependencies are newer.
  • Makes invalidation handling much easier and reliable for complex dependencies

New ProxyManager class:

• Supports loading multiple cache entries at once, making loading much faster when accessing multiple objects
• Tries to minimize the number of cache reads and writes for performance
• Enables separate cache entries to contain references to the same cache entry objects
  • Enables circular references and loading a cache entry once loads it for every object that references it

New DBDataManager class:

• Allows loading data from the database for multiple cache entries at once. Useful when the cache entries make use of the same database data.

Fixes a part of #888

Testing

What type of test did you run?

• Accessibility test using the WAVE extension.
• Django unit tests.
• Selenium tests.
• Other test. (Add a description below)
• Manual testing.

Accessed most of the major pages while debugging,

Did you test the changes in

• Chrome
• Firefox
• This pull request cannot be tested in the browser.

Translation

• Did you modify or add new strings in the user interface? (Read about how to create translation)

Programming style

• Did you follow our style guides?
• Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

• documents inside the doc directory.
• README.md.
• Aplus Manual.
• Other documentation (mention below which documentation).

Is it Done?

• Reviewer has finished the code review
• After the review, the developer has made changes accordingly
• Customer/Teacher has accepted the implementation of the feature

[lainets]

A new issue to be added if this is merged: exercise.viewbase.ExerciseMixin.get_summary_submissions passes self.feedback_revealed to ExercisePoints but I believe self.is_course_staff would be enough (wouldn't require DB access then). This needs to be investigated. This would also make it nearly identical to the cached_points property in the get_cached_points method: only difference is that cached_points can technically be for a non-submittable exercise. Would also need to investigate whether these two can be merged (is the exercise always submittable?)

Already did this as part of additional optimizations.

[lainets]

Performance changes

I'm a little worried about the large variances in the load times. Sometimes it seems to be very slow. I'm not sure if this only happens with the new changes but it does seem like that. It might be caused by the garbage collector activating during the load.

Synopsis

• No apparent change to course configure time.
• Load time: Slower than before and very much dependent on the number of separate cache entries to be loaded (i.e. the number of LearningObjects on the course). This means the impact is seen more on large courses. CachedContent went from ~5ms to ~25ms on O1, and points went from 25-30ms to ~44ms. Sometimes it takes considerably more time (may be due to garbage collector).
• Generation time:
  • maybe a little faster for cached content
  • seemingly a little slower for cached points for few submissions
  • seemingly faster for cached points for a lot of submissions

O1 benchmarks (on minus)

The decimal point numbers are in seconds. 4-8ms ~5ms means that the value varied from 4 to 8 ms but (estimated) median took 5ms.

BEFORE (pymemcache):
content: - generate: 0.740134373

• load: 4-8ms ~5ms
  Points with no submissions:
• generate: 0.15735532900000004
• load: 21-27ms ~27ms
  Points with few hundred submissions:
• generate: 0.609012841
• load: 20-31ms ~30ms

BEFORE (pylibmc):
content:

• generate: - 0.563846949 0.9350565309999999
• load: 4-8ms ~7ms
  Points with no submissions:
• generate: 0.14251463899999806 0.10672521400000001
• load: 0.14770852599999973 ~19ms
  Points with few hundred submissions:
• generate: 0.8493996490000004 0.588477317 0.706424712
• load: ~24ms

AFTER (pymemcache):
content:

• generate: 0.627027349 0.6389316300000001
• load: 60-200ms ~60ms
  Points with no submissions:
• generate: 0.222431614 0.25517966700000017
• load: 80-200ms ~80ms
  Points with few hundred submissions:
• generate: 0.46071420100000005 0.5961609270000001
• load: 80-200ms ~86ms

AFTER (pylibmc):
content:

• generate: 0.66984569 0.46586513700000004
• load: ~25ms
  Points with no submissions:
• generate: 0.1542314090000001 0.13666057200000004
• load: 40-270ms ~44ms
  Points with few hundred submissions:
• generate: 0.5559999369999999 0.5619267869999999
• load: 40-270ms ~44ms

[markkuriekkinen]

Unfortunately, I'm not able to review this properly since we have run out of time. This pull request is extremely complex and it is scary. A thorough review would take a lot of time. There are major risks in making very complex changes in the core of an old system. If something were broken, would we ever be able to locate and fix the issue? Do we even notice the malfunction before it has caused too much damage?

It is hard to evaluate whether the pull request is really good and well-designed or fundamentally broken. It is so complex that I can't quickly make much sense of it.

Anyway, you may finish this and do the final git-rebase and such. We will need to merge this soon so that it doesn't fall behind the master branch. Fixing git conflicts would be awful with such a massive pull request.

[markkuriekkinen]

I forgot to add in the previous message that it is great that you have written long git commit messages. They help others to understand all this.

[lainets]

If something were broken, would we ever be able to locate and fix the issue?

It might take some time but I do think you will be able to fix any issues. In any case, there is always the option of reverting the changes.

To note: if there ever comes a need to revert these changes, you might want to revert to c0f447c or 5778401 if the former commit has the issues. If even the latter commit has issues, you can just revert the whole thing. The idea is that the second linked commit contains the change from a dict to a class which is the big change in the cache interface. This makes reverting any changes made after this PR is merged back to the old cache much easier. The first linked commit contains some should-be harmless simplification of the code on top of the second linked commit which is why it should be left alone if possible.

Do we even notice the malfunction before it has caused too much damage?

The only way I can see this change causing serious damage is if the point calculation is wrong because then the actual points given by the all results page is different from the points shown on UI but I think this would be noticed very fast (and I didn't see any such issues in any of my testing). The thing about the cache is that it mostly just affects the UI. The most that can happen is that it shows wrong points, points that should be hidden, or pages are listed in the course front page that shouldn't be there (or aren't listed when should). The other option is that there is a bug that crashes a particular view (most likely would be because I forgot to change the dict access notation to the property access notation somewhere) but this isn't a huge issue as the fix should be simple, and all the major views seem to work based on my testing.

Biggest concerns are exactly that I forgot to swap some dict access to a object property access or the cache TransactionManager somehow misbehaves (test_nested_transactions for it seems to sometimes randomly fail on github, however such nesting of transactions doesn't really happen in the code and other tests pass fine). The latter will at most fail to save (highly unlikely as this is never done inside a transaction anyway) or invalidate a cache entry, so nothing major. It is also easy to remove by just accessing the cache directly in lib/cache/cache.py instead of through the TransactionManager. This will, however, add back the possibility of the cache not being invalidated correctly after the transaction in course configuring.

It is hard to evaluate whether the pull request is really good and well-designed or fundamentally broken. It is so complex that I can't quickly make much sense of it.

I can confidentally say that it at least isn't fundamentally broken. I think the design is good given the constraints on performance. Most of the complexity comes from performance optimizations, so it could be "better" (i.e. simpler in this case) if we didn't care about performance.

Therea are only two things I don't like about the code: the modifiers given to the cache objects and how the show_unrevealed is handled in the generation code (two attributes for each revealable value) but it was the best way I could think of implementing show_unrevealed without having to save the data twice in the cache. Though these are pretty minor things.

Anyway, you may finish this and do the final git-rebase and such.

I have done the final rebase.

[lainets]

Some basic benchmarks of fully loading (including exercises) a particular chapter page with multiple exercises on O1 on my pc:

• 2,25-2,6s before cache changes and without the optimization at the end of this PR
• 3,0-3,2s after cache changes and without optimization (this value might actually be old but it should be a little higher than the above)
• 2,00-2,2s after cache changes and with optimizations
• 0,95-1,1s after cache changes and with optimizations and loading the exercises in parallel instead of sequentially (not included in this PR)

[lainets]

Don't merge this yet. I noticed a module model answer related optimization that I should do before that

[lainets]

This is now done.