Performance #1035
Conversation
adamruzicka
force-pushed
the
perf
branch
2 times, most recently
from
7adeb14 to
671573e
Compare
|
As a non-admin user, looking at a job with 5000 hosts, this fix brings the time to first full draw from ~43 seconds to ~15 seconds |
Lukshio
left a comment
Lukshio
left a comment
There was a problem hiding this comment.
Hi, I've finished first round of the review, and found some things to discuss.
| dispatch(fetchData); | ||
| export const getJobInvocation = url => dispatch => { | ||
| stopJobInvocationPolling(); | ||
| fetchJobInvocation(dispatch, url, { include_permissions: true }); |
There was a problem hiding this comment.
I would be very careful about calling with inlcude_permissions only once. We can use it in each request, the difference between response with and without this information should not be more significant. After that we can simplify calls by removing this getJobInvocation. All informations should be already in the repeating calls.
There was a problem hiding this comment.
One of the goals here was to reduce the number of times we re-load rarely changing information we already have.
We can use it in each request, the difference between response with and without this information should not be more significant.
The assumption was that the permissions shouldn't really change as the job runs, even though technically they can and recalculating those every second is a waste in majority of cases.
the difference between response with and without this information should not be more significant.
Yes, at the same time, it is yet another thing the backend has to compute.
With that being said, I don't feel strongly about this. We may find out that the savings coming from this are negligible and then we can very well favor the code simplicity over this.
There was a problem hiding this comment.
On the other hand, I assume that when user privileges changes, or user logs out, server won't respond for this recurring api request, so there still will be permissions check on server side. So we can omit it on frontend.
There was a problem hiding this comment.
server won't respond for this recurring api request, so there still will be permissions check on server side
Backend always checks permissions, no matter what.
So we can omit it on frontend.
I was under the impression we load these so that we know which action elements can be shown as active. Yes, we could skip that, make everything appear to be clickable and let the backend reject what the user can't do, but then the user experience wouldn't be all that great.
There was a problem hiding this comment.
If we will check it in first request, it will render buttons correctly, so there won't be any issue, and if something changes (user log out, change permissions etc.) request will fail and user will be redirected or will have to reload, that will send new first request and receive updated privileges. So this should work fine.
| statusLabel === STATUS.FAILED || | ||
| statusLabel === STATUS.SUCCEEDED || | ||
| statusLabel === STATUS.CANCELLED; | ||
| const autoRefresh = task?.state === STATUS.PENDING || false; |
There was a problem hiding this comment.
As this is bigger refactor. We have some inconsistency across the states like this one. Auto refresh is in status pending, but it should be also in status running which we are not checking. Same issue with the Create Report button, where user can create report when the job is not finished yet. Could we introduce new state RUNNING and adjust this check for it? Or convert pending state to array and then check it like that.
If you consider it as another task, I'm okay with that.
Create Report bug: https://redhat.atlassian.net/browse/SAT-44552
There was a problem hiding this comment.
I would agree that the frontend should cover all the possible values that it might get from the backend one way or another. Filed https://projects.theforeman.org/issues/39360
There was a problem hiding this comment.
I would prefer to solve the task after this refactor, so we don't change each others part, wdym?
@hosts.to_a materializes all the hosts. Previously it read all the hosts in the job, with this change it reads only the current page.
…on details The extra data it gave doesn't seem to be used anywhere in the job invocation details page.
It's a bit easier on the eyes
Replace setInterval-based polling (via withInterval) with a setTimeout chain that only schedules the next request after the previous one completes or fails, preventing request pileup when the server is slow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The include_permissions param causes the backend to instantiate an Authorizer and check 3 permissions on every request. These are user permissions that don't change during a page session, so fetch them once on the initial request and skip them on subsequent polls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
updateJob fetched /api/job_invocations/:id into the UPDATE_JOB Redux key, but no selector ever read from that key. The polling loop already refreshes the job state via JOB_INVOCATION_KEY every second, so these extra requests after cancel/abort/recurring actions were wasted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The second useEffect called filterApiCall() unconditionally whenever any dependency changed, and again when statusLabel changed — causing 2-3 identical requests on initial load. Track previous values for both initialFilter and statusLabel so filterApiCall fires only once per actual change. Also move setStatus(RESOLVED) inside the JOB_INVOCATION_HOSTS branch of handleResponse so the LIST_TEMPLATE_INVOCATIONS fetch (which doesn't populate table data) doesn't prematurely flip the table out of its loading state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The detail page fetched /foreman_tasks/api/tasks/:id solely to read available_actions.cancellable. Add cancellable to the task child in the job invocation response instead, eliminating a separate API call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
handleSuccess unconditionally rescheduled the next poll, so the useEffect's stopJobInvocationPolling call raced against in-flight requests — the cleared timeout was immediately replaced by the completing fetch. Check the response data inside handleSuccess instead, and simplify the effect to only handle unmount cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Includes #1034
Opening as a draft until I actually read through it. Whether the individual fixes should be included is up to debate.
I'd expect these two to yield the biggest benefits:
The rest are sort of nice to haves