-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Parallelize and simplify the asset processing loop. #21701
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
base: main
Are you sure you want to change the base?
Conversation
In the future, can we have some way of a) polling memory usage and b) predicting memory usage and then gating the task spawning on that? For now, can we add a run-time configurable maximum number of active tasks? |
194cc07 to
f9f2112
Compare
|
@alice-i-cecile I think my initial assessment was wrong. The previous implementation was no better than this PR for the initial processing case - previously it ran all the tasks concurrently rather than in parallel, but that still means that we're using up all that memory while all the tasks are running. I think we should leave any sort of memory control to a future PR, and I don't really want to complicate this PR with like a semaphore. Thoughts? |
|
Also CI is just broken by #21702, so that's not relevant. |
a96570d to
073d971
Compare
…s can solve that.
…easier to start and stop asset sources.
…s, and use a select loop to listen for them.
073d971 to
8d2347c
Compare
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.
Makes sense to me and feels like a good change. My first thought was also "what happens if you load 'too many' at once" with the new parallelism and no controls, but that has been addressed and there's plenty of time before the next release for that to be tested and dealt with if necessary.
Objective
check_reprocess_queuewhich would only be checked after all the current events have been handled.Solution
Approximately throw everything out. The asset processor now does these things:
So this parallelizes event processing from asset sources, parallelizes processing each asset, and (IMO) makes the whole processing loop much simpler.
Also I think it's funny that parallelizing could make things simpler lol.
Caveats
process_assetsandlisten_for_source_change_events. My guess is these were public so that users can call them outside the context of a running app? I'm not entirely sure. I think this needs to be rethought though if that's the case. For one, a running app currently will not be gated on processing from another app, meaning things will probably get out-of-sync very easily. If need be, I think we can bring this back fairly straight forward. There also isn't a migration guide since there's nothing to migrate to here.Testing