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

use process pool to compile faster #2377

Merged
merged 4 commits into from
Jan 12, 2024
Merged

use process pool to compile faster #2377

merged 4 commits into from
Jan 12, 2024

Conversation

jackie-pc
Copy link
Contributor

@jackie-pc jackie-pc commented Jan 9, 2024

  • On platforms that support it (Mac, Linux), use a forking ProcessPoolExecutor instead to leverage multiple CPU cores. Compilation (especially for a site that has many pages) is CPU bound. Python's faux threads don't help very much (for reflex-web, setting max threads to 1 results in same performance prior).
  • Some gymnastics required to use processes, to workaround non-picklable task arguments (like components). We make the strong assumption that we use these arguments in a read only manner, and return strings (path + compiled output text).

We also do some general cleanup - for steps that are not being parallelized, it should run outside of the executor context manager.

As an aside, we stop using multi-threading to write out final compilation output to file system. Even for a large project like reflex-web, that part takes about 200ms on my laptop (Macs are fancy, but pretty much everyone has SSDs).

Before (reflex-web):
image

After (reflex-web):
image

@jackie-pc jackie-pc marked this pull request as ready for review January 10, 2024 00:34
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome it's actually faster

need to merge main to get it working with the latest reflex-web radix changes

reflex/app.py Outdated
@@ -82,6 +84,118 @@ def default_overlay_component() -> Component:
return connection_modal()


class ExecutorSafeFunctions:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like this class belongs in reflex/compiler/utils.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. IMO the gymnastics involved are little icky so encapsulating as much as we can would be good. More to do in future on that front.

reflex/app.py Outdated
Comment on lines 119 to 131
@staticmethod
def compile_page(route: str):
"""Compile a page.

Args:
route: The route of the page to compile.

Returns:
The path and code of the compiled page.
"""
return compiler.compile_page(
*ExecutorSafeFunctions.COMPILE_PAGE_ARGS_BY_ROUTE[route]
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a particular reason to go for the staticmethod over classmethod when these do in fact access members of the class?

seems like that would have less code duplication and make it easier to subclass/monkeypatch if some behavior modifications were needed downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason. These things started out as full global vars or functions, in fact! The key property is only that the forked child process is able to get at the data / functions correctly. I can try to see if @classmethod works as well.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

@picklelo picklelo merged commit a8756cb into main Jan 12, 2024
44 of 45 checks passed
picklelo added a commit that referenced this pull request Jan 22, 2024
@picklelo picklelo deleted the jackie-faster-compile branch January 30, 2024 03:09
jackie-pc added a commit that referenced this pull request Jan 31, 2024
jackie-pc added a commit that referenced this pull request Feb 13, 2024
masenf added a commit that referenced this pull request Mar 5, 2024
masenf added a commit that referenced this pull request Mar 12, 2024
masenf added a commit that referenced this pull request Mar 13, 2024
masenf added a commit that referenced this pull request Mar 16, 2024
* Revert "Revert "Revert "Revert "use process pool to compile faster (#2377)" (#2434)" (#2497)" (#2595)"

This reverts commit 6b6eea4.

* Adjust number of operations for more correct progress bar

* app: recognize REFLEX_COMPILE_PROCESSES and REFLEX_COMPILE_THREADS

Control whether multiprocessing is used and the number of processes or threads
that should be used.

This will allow users to opt-in to the new, potentially hazardous,
multiprocessing mode, which results in much faster compiles, but has already
been reverted 4 times. Lets leave the code in this time, but use the thread
pool executor by default.

Limiting the number of threads or processes to 1 can also aid in debugging
issues that arise during compile time.

* Allow REFLEX_COMPILE_PROCESSES=0 to trigger multiprocessing with auto workers
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.

None yet

3 participants