-
Notifications
You must be signed in to change notification settings - Fork 6
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
GN-4472: Optimize loading of meeting page #558
Conversation
extends the adapter so it checks for defaultPageSize properties on hasMany relationships refactor behandeling component to fetch participants using the relationship, which now insta-resolves because of the includes higher up
this allows for optimization with includes. A high pagesize is set to avoid issues.
- load possibleParticipants on demand (only needed when attendee modals are openened) - load treatments in batches, with batch 2 and up running concurrently (batch 1 runs first to also get the total amount of treatments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a behandeling (treatment) always have a documentContainer? Sometimes there is a null check and sometimes there isn't. But this may be that it does always have it in certain flow/pages and not in other.
I tried to break stuff, but honestly it seems to work as expected as far as I can tell. Lots of buttons and things interacting though, so I might have missed something too. I did look at the code and tried to test those specific places a second time in detail.
Some small comments, but none about breaking code. just cleanup, comments and some suggestions.
Not related to the ticket:
the defaultPageSize is already better than all the pageSize
's in queries (where you have to remember to do this too). Still a hack to make it "big enough hopefully" though.
Really with how await something.relationship
doesn't paginate, there are two options.
Or you always paginate (meaning always use query) for relationships that could have a ton of items
Or never paginate, meaning always fetch everything, for a specific relationship that should be limited (using dot syntax).
Anything inbetween like it is now (be it with 20 or a set pageSize) might introduce bugs or fall in the middle of what is desired.
I don't see why you'd ever want to use the dot syntax (which uses findHasMany) if you don't want all items. If you do only want to have "the first few", a query is much more explicit.
So I honestly don't see why it wouldn't make sense to make dot-syntax => fetch everything (so findHasMany => returns everything). Query => pagination, where you can specify page size if needed. Although having a bigger default pagesize settable in model would be good for query too.
Co-authored-by: x-m-el <[email protected]>
Co-authored-by: x-m-el <[email protected]>
Overview
While the linked ticket was resolved with a config change on the prod server, in the meantime
I also tackled one of the two underlying causes: a woefully inefficient data loading strategy
for the meeting page. (the other being a bug in mu-cl-resources relating to cache invalidation, which is the reason treatments can no longer be cached)
From my own unscientific tests, these changes reduce the load time of a 75 item page from
over 1 min down to around 10 seconds.
But more importantly, it also reduces the amound of requests made from over
1000 down to just over 100.
This was ultimately the cause of the reported issue: mu-cl-resources choking on the sheer amount
of simultaneous requests.
I've tried to limit my refactoring to data loading only. I've used ember-resources where they
were easy to fit in, but didn't go out of my way to use them.
One of most important changes is the extension to the ApplicationAdapter. It allows for a new
option when specifying a
hasMany
relationship on a model:defaultPageSize
. As the name says,this makes it so the specified pagesize is used when loading the items of the relationship.
This allows us to use
await foo.relationshipItems
without worrying about pagination issues (orat least, it gives us the tools to make it feasible.)
In the pr, a few relationships have been updated this way. If you go look at the
store.query
callsthey now replace, you'll see that these arbitrary "limits" are not regressions, we were already
avoiding pagination by using a large pagesize in multiple locations.
The reason for doing this, is that using relationships allows us to optimize the requests by
using the
include
query param when loading the parent model.NOTE: even if you include a relationship, you still have to await it.
It'll resolve immediately, but it might not, in some cases (and in any case, its a Proxy until
you await it). It is purely an optimization, and does not change how you use the relationships.
Some other notable changes include splitting up the data loading in the MeetingForm component,
to allow for concurrency of the big chunks (the participants and the treatments).
The treatments themselves are loaded in 20 item batches, with batch 2 and up loading concurrently.
(This saves quite a lot of time on large pages, these are by far the slowest requests).
You'll note that most of the remaining requests (in fact, 75 of them, on a 75 item page)
are fetches of the attachments of document containers.
I couldn't get the include to work for those, likely because none of the containers
actually had attachments, and that may result in ED refusing to cache it.
This should be no problem though, the requests are quick and will run mostly concurrently in http2,
and anyway it's not a visual blocker, so the user will likely barely notice.
connected issues and PRs:
GN-4472 for tracking
GN-4466 for context
Setup
just run it. Recommended to use a large meeting >50 items when testing.
How to test/reproduce
PLEASE test this thoroughly. It is highly likely I missed something, and broke some reactivity
somewhere. Make agressive changes, check data consistency when reloading the page, look out for
reactivity issues (e.g. open modal to change something, close modal, it hasnt updated in the
overview)
I've done my best to keep the commits to a sensible size and include meaningful messages.
It might help you to go in sequence.
Challenges/uncertainties
Checks PR readiness