Skip to content
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

LDrawLoader: parallelize parts library downloads to improve load times #22253

Merged
merged 13 commits into from
Aug 3, 2021

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Aug 3, 2021

Merge #22249 first

Description

This PR focuses on parallelizing file downloads when loading models from the LDraw Parts Library. There's a lot of rearranging that happened so the diff looks pretty large. The big changes are:

  • Separate out the cache into a class so the url retry functionality can be more easily encapsulated.
  • Change a lot of scoped functions and callbacks with stack state to use promises and async which makes it easier to track what's going on and write this kind of asynchronous code.
  • Remove reliance on the loader.parseScopesStack in favor of bookkeeping the parent scope on the child scope directly and passing scopes in as function arguments so asynchronous code doesn't muck up the current scope context.
  • A number of local functions specifically in processObject have been removed because promises simplify things.
  • finalizeObject has been changed to take the parseScope to finalize and moved to be a class function so the parent scope can guarantee the objects are processed in the right order to avoid messing up the construction steps.

With this change loading the Imperial Star Destroyer Lego model goes from a load time of a little over 4 minutes down to around 1 minute 25 seconds. I think there is still room for improvement to be had by caching the parsed file contents rather than parsing the text content over and over every time a file is reused (which might be one of my upcoming PRs 😁).

@mrdoob This PR adds a few arrow functions to the class -- I'm not sure what you're feelings are on using them in the project but generally I find them to be easier to look at and simplify things when working with an outer "this" pointer. If you'd like me to remove them let me know.

For testing you can check out the live link to the ldraw example here (which still is just using the prepacked models).

image

cc @yomboprime

@gkjohnson gkjohnson changed the title Ldraw parallelize LDrawLoader: parallelize parts library downloads to improve load times Aug 3, 2021
@mrdoob mrdoob added this to the r132 milestone Aug 3, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 3, 2021

@mrdoob This PR adds a few arrow functions to the class -- I'm not sure what you're feelings are on using them in the project but generally I find them to be easier to look at and simplify things when working with an outer "this" pointer. If you'd like me to remove them let me know.

I think it makes sense to use these when requiring a "this" pointer 👍

@gkjohnson gkjohnson marked this pull request as ready for review August 3, 2021 15:09
@gkjohnson
Copy link
Collaborator Author

I think it makes sense to use these when requiring a "this" pointer 👍

Okay I changed a couple of the arrow functions to classic style where this isn't needed!

@mrdoob mrdoob merged commit aa15803 into mrdoob:dev Aug 3, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 3, 2021

Thanks!

@gkjohnson gkjohnson deleted the ldraw-parallelize branch August 3, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants