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

Server would occupy a lot of memory when method is not async #980

Closed
ZackJiang21 opened this issue Jun 22, 2020 · 10 comments
Closed

Server would occupy a lot of memory when method is not async #980

ZackJiang21 opened this issue Jun 22, 2020 · 10 comments

Comments

@ZackJiang21
Copy link

I just read the source code of starlette, and I think I found reason why it's occupying so much memory
The problem is in starlette.routing.py methodrequest_response()


```python
def request_response(func: typing.Callable) -> ASGIApp:
    """
    Takes a function or coroutine `func(request) -> response`,
    and returns an ASGI application.
    """
    is_coroutine = asyncio.iscoroutinefunction(func)

    async def app(scope: Scope, receive: Receive, send: Send) -> None:
        request = Request(scope, receive=receive, send=send)
        if is_coroutine:
            response = await func(request)
        else:
            response = await run_in_threadpool(func, request)
        await response(scope, receive, send)

    return app
  
 
async def run_in_threadpool(
    func: typing.Callable[..., T], *args: typing.Any, **kwargs: typing.Any
) -> T:
    loop = asyncio.get_event_loop()
    if contextvars is not None:  # pragma: no cover
        # Ensure we run in the same context
        child = functools.partial(func, *args, **kwargs)
        context = contextvars.copy_context()
        func = context.run
        args = (child,)
    elif kwargs:  # pragma: no cover
        # loop.run_in_executor doesn't accept 'kwargs', so bind them in here
        func = functools.partial(func, **kwargs)
    return await loop.run_in_executor(None, func, *args)

My rest interface is not async, it will run in loop.run_in_executor, but starlette do not specify the executor here, so the default thread pool size should be os.cpu_count() * 5, my test machine has 40 cpus so I should have 200 threads in the pool. And after each request it will not release the object in these threads, unless the thread be reused by next request, which will occupy a a lot of memory. Especially when I wrap a large deep learning model in the server.

My question is could we make the thread pool size configurable?

@raphaelauv
Copy link

It seams that there is a limitation to 32 threads in the source code

https://github.com/python/cpython/blob/7d9d25dbedfffce61fc76bc7ccbfa9ae901bf56f/Lib/concurrent/futures/thread.py#L136

@ZackJiang21
Copy link
Author

It seams that there is a limitation to 32 threads in the source code

https://github.com/python/cpython/blob/7d9d25dbedfffce61fc76bc7ccbfa9ae901bf56f/Lib/concurrent/futures/thread.py#L136
The link u pasted is in python 3.8, but if run on python 3.7 or lower version, it does not have the limitation. So I suppose it still make sense to do the limitation in startelle.

@tomchristie
Copy link
Member

My question is could we make the thread pool size configurable?

No - we ought to have sensible configurations here on behalf of our users, rather than adding to the number of things they need to think about.

If anyone's motivated enough to dig into clear justifications that the system defaults are appropriate for us, and has an actionable change that we oughta make, then let's consider that. Otherwise, let's just leave this as it is.

@aminalaee
Copy link
Member

aminalaee commented Feb 3, 2022

I think this was for before migrating to anyio.
We now rely on anyio default pool size. Last time I checked the default pool size for asyncio is 40.

I guess this is still configurable outside of Starlette, so we can keep this closed.

@tomchristie
Copy link
Member

Thanks @aminalaee 👍🏼

@Kludex
Copy link
Member

Kludex commented Feb 3, 2022

It's worth mentioning that you can modify the default capacity limiter on anyio. 👍

@adriangb
Copy link
Member

adriangb commented Jul 8, 2022

It's worth mentioning that you can modify the default capacity limiter on anyio. 👍

I don't think this is true

@Kludex
Copy link
Member

Kludex commented Jul 8, 2022

@adriangb
Copy link
Member

adriangb commented Jul 8, 2022

Yup you're right, it can be modified, I thought you meant replace 😅

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

No branches or pull requests

6 participants