Re: Performance does not scale when increasing file upload sizes

['Noxx' via Django developers (Contributions to Django itself)]

Hi all,

I've played around a bit and would like to give you a heads up:

WSGI and ASGI have only differences in how they parse incoming requests.
As WSGI is synchronous the body is already provided as a ByteIO stream 
which is set directly on the HTTPRequest (wrapped as a LazyStream).

On ASGI the body is sent in chunks. As the base of HTTPRequest - and this 
includes how to parse the body, POST, FILES etc. - is the same for ASGI and 
WSGI, those chunks need to be joined together prior to any other processing.
to do this, all chunks are currently written into the SpooledTemporaryFile 
mentioned in my previous post and this file is then provided as a stream to 
the HTTPRequest.
As already written, this workaround to get all chunks into one stream is 
very slow for requests which are bigger in their size as the temporary file 
is spooled to disc in the worst case.

To workaround this issue, I've tried to delay the reading of the ASGI queue 
to get the next chunk (called "message" in ASGI) until the HTTPRequest 
really needs it.
As the HTTPRequest/MultiFileParser works on a stream to read and parse its 
body, the idea is to use the ASGI queue as a stream-like object and to 
directly read it.
This would eliminate the need to create a temporary file as a "proxy".

When I was in the process of implementing this interface, I've recognized 
that the package a2wsgi already handles such use-case:
https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/wsgi.py

I've added a2wsgi as a dependency and change the ASGI handler a little bit.
The changes can be reviewed on my fork: 
https://github.com/Flauschbaellchen/django/commit/006bf2d5c0b82092524ee9b65ef919637165ee01

Timings:
In my test environment, the timing went from 4s (sending)/16s (waiting) to 
6s/5s.
Sending is a bit slower. This is because prior to the change the chunks 
have been read directly into the temporary file and now the parser is doing 
its business between reading new chunks.
However, those chunks are directly parsed and feed into the upload handlers 
which dramatically improves the performance on the server side.
Using gunicorn+uvicorn the waiting time went down to ~600ms. I don't know 
why runserver/daphne is so slow. I'll check this within the next days.

I've attached a new flame graph representing my changes.
The right-most block can be ignored - this is already the next GET request 
of the admin panel after saving the object.

What's next?
I would appreciate any feedback on my changes.
Is this the right direction?
What about the newly introduced dependency? - It would also be possible to 
implement/copy the code on the Django side to eliminate this dependency. 
However, on my opinion, doing things twice is never a good idea...
Can anyone test against other environments/ASGI servers? Maybe I've 
overlooked some edge cases.

The current changes depends on the version 4.0.4 of Django as on the main 
branch small changes happend to the ASGI handler.
I'll rebase my changes prior to submitting a PR.

With kind regards :)

On Wednesday, May 11, 2022 at 7:47:10 PM UTC+2 Adam Johnson wrote:

> Ferran - No, this is the right place. Noxx is suggesting a change to 
> Django.
>
> Noxx - it seems reasonable to improve Django here. You mentioned ASGI - 
> did you check what the WSGI paths do? It may be that the newer ASGI code is 
> simply less optimal.
>
> Would it be possible to reduce the workflow to a single write call?
>>
>
> This is indeed the question. Could you investigate further and perhaps 
> come up with a patch? You're likely "the world expert" on this part of 
> Django at this point, after all the profiling you've done.
>
>
> On Wed, May 11, 2022 at 5:44 PM Ferran Jovell  wrote:
>
>> This is still the wrong place. Try django-users mailing list. 
>>
>> El dc., 11 de maig 2022, 17:53, 'Noxx' via Django developers 
>> (Contributions to Django itself)  va 
>> escriure:
>>
>>> Story
>>>
>>> Our application supports many file uploads.
>>> In normal circumstances, those files are only a few megabytes to of 
>>> around 100MB.
>>> However, some uploads can be several GB in size (10-15GB and up).
>>>
>>> I know, that file uploads with such a size can be handled differently,
>>> e.g. using a seperate endpoint, allowing slow requests to be served 
>>> seperately from the application etc.
>>> and only to reference those files in a final request.
>>> This solution would introduce some other problems regarding 
>>> house-keeping, non-atomic requests etc, thus I want to ignore this 
>>> workaround for now.
>>> Performance impact: 
>>>  
>>>
>>> The impact can be best observed when uploading a file which is bigger in 
>>> it's size, e.g. 1GB+.
>>> On my maschine it takes the client around 700ms to send the request to 
>>> the application, but than waits around 1.5s for the final response.
>>> Of course, those numbers are dramatically influenced additionally in 
>>> production by storage speed, fi

Re: Performance does not scale when increasing file upload sizes

[Carlton Gibson]

Hi Noxx.

This is interesting, thanks for the write-up.

As a bit of history, initially ASGIRequests would read the whole body into
memory, which was fine for an initial version, but led quickly enough to
resource concerns. Andrew followed up with the SpooledTemporaryFile
approach, as a nice next-step improvement on that. There **was** at the
time some initial work on wrapping the receive queue to consume the body
on-demand, but with the temp-file solution in hand that never got pushed
through. I can very well imagine folks in ASGI-land have had time to get
that right now. (So exciting to look at!)

I'd very much want to dig into a dependency before deciding to take one on.
I suspect it's not **that** complex, so it's likely something we'd want to
re-implement to fit our needs exactly. (But we can see on review...)

Please do open a PR when you're ready.

Kind regards,
Carlton

On Tue, 17 May 2022 at 15:20, 'Noxx' via Django developers (Contributions
to Django itself)  wrote:

> Hi all,
>
> I've played around a bit and would like to give you a heads up:
>
> WSGI and ASGI have only differences in how they parse incoming requests.
> As WSGI is synchronous the body is already provided as a ByteIO stream
> which is set directly on the HTTPRequest (wrapped as a LazyStream).
>
> On ASGI the body is sent in chunks. As the base of HTTPRequest - and this
> includes how to parse the body, POST, FILES etc. - is the same for ASGI and
> WSGI, those chunks need to be joined together prior to any other processing.
> to do this, all chunks are currently written into the SpooledTemporaryFile
> mentioned in my previous post and this file is then provided as a stream to
> the HTTPRequest.
> As already written, this workaround to get all chunks into one stream is
> very slow for requests which are bigger in their size as the temporary file
> is spooled to disc in the worst case.
>
> To workaround this issue, I've tried to delay the reading of the ASGI
> queue to get the next chunk (called "message" in ASGI) until the
> HTTPRequest really needs it.
> As the HTTPRequest/MultiFileParser works on a stream to read and parse its
> body, the idea is to use the ASGI queue as a stream-like object and to
> directly read it.
> This would eliminate the need to create a temporary file as a "proxy".
>
> When I was in the process of implementing this interface, I've recognized
> that the package a2wsgi already handles such use-case:
> https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/wsgi.py
>
> I've added a2wsgi as a dependency and change the ASGI handler a little bit.
> The changes can be reviewed on my fork:
> https://github.com/Flauschbaellchen/django/commit/006bf2d5c0b82092524ee9b65ef919637165ee01
>
> Timings:
> In my test environment, the timing went from 4s (sending)/16s (waiting) to
> 6s/5s.
> Sending is a bit slower. This is because prior to the change the chunks
> have been read directly into the temporary file and now the parser is doing
> its business between reading new chunks.
> However, those chunks are directly parsed and feed into the upload
> handlers which dramatically improves the performance on the server side.
> Using gunicorn+uvicorn the waiting time went down to ~600ms. I don't know
> why runserver/daphne is so slow. I'll check this within the next days.
>
> I've attached a new flame graph representing my changes.
> The right-most block can be ignored - this is already the next GET request
> of the admin panel after saving the object.
>
> What's next?
> I would appreciate any feedback on my changes.
> Is this the right direction?
> What about the newly introduced dependency? - It would also be possible to
> implement/copy the code on the Django side to eliminate this dependency.
> However, on my opinion, doing things twice is never a good idea...
> Can anyone test against other environments/ASGI servers? Maybe I've
> overlooked some edge cases.
>
> The current changes depends on the version 4.0.4 of Django as on the main
> branch small changes happend to the ASGI handler.
> I'll rebase my changes prior to submitting a PR.
>
> With kind regards :)
>
> On Wednesday, May 11, 2022 at 7:47:10 PM UTC+2 Adam Johnson wrote:
>
>> Ferran - No, this is the right place. Noxx is suggesting a change to
>> Django.
>>
>> Noxx - it seems reasonable to improve Django here. You mentioned ASGI -
>> did you check what the WSGI paths do? It may be that the newer ASGI code is
>> simply less optimal.
>>
>> Would it be possible to reduce the workflow to a single write call?
>>>
>>
>> This is indeed the question. Could you investigate further and perhaps
>> come up with a patch? You're likely "the world expert" on this part of
>> Django at this point, after all the profiling you've done.
>>
>>
>> On Wed, May 11, 2022 at 5:44 PM Ferran Jovell  wrote:
>>
>>> This is still the wrong place. Try django-users mailing list.
>>>
>>> El dc., 11 de maig 2022, 17:53, 'Noxx' via Django developers
>>> (Contributions to Django itself)  va
>>> escriu

Experience with black formatting in django

[Tom Aldcroft]

The astropy project is currently considering implementing code formatting 
with black, much as Django did in 2019. I was happy to see the case for 
transitioning to black formatting in Django DEP-0008 
(https://github.com/django/deps/blob/main/final/0008-black.rst). I think it 
captures the key points both pro and con, along with the inevitable lack of 
complete consensus in a large project.

My goal in writing is to request feedback from Django developers about how 
this transition has impacted django development and community engagement in 
the three years since the transition. 

Has there been specific feedback from contributors (both experienced and 
new) related to black autoformatting?

   - Ease of actually running black and meeting the black-format standard 
   for PRs. Have there been technical problems that required assistance?
   - Style considerations?

Have most core developers accepted or embraced the black style at this 
point? The DEP included some testimonials to that effect, but some of us 
wondered if those represented a fair sampling of the community.

Are there any regrets or things you would change?

Do you have any advice for the astropy project?

Any feedback from your experience would be most appreciated.

Regards,
Tom (@taldcroft on GitHub)

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/33b9a27e-2a0c-4e7c-ba2b-c5072a87073an%40googlegroups.com.

Django 4.1 alpha 1 released

[Carlton Gibson]

Details are available on the Django project weblog:

https://www.djangoproject.com/weblog/2022/may/18/django-41-alpha-1-released/

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAJwKpyQmCB6q__p167GcmaH8dBT%3D-jSbtr2LLb_E63E9QRQUXQ%40mail.gmail.com.

Re: Performance does not scale when increasing file upload sizes

['Noxx' via Django developers (Contributions to Django itself)]

Hi Carlton,

thanks for your response.
I've opened up a PR: https://github.com/django/django/pull/15704

I've decided to moved the "ASGIStream" wrapper code into the Django code 
and get rid of the dependency as it is only a small part of it.
My version of the code was nearly the same as the one from a2wsgi so I used 
the latter one (I think that their version is a bit more battle-proofed) 
and added the http.disconnect exception which was missing.
However, I do not know what would be an appropriate way forward:
Leave it as it is, add a "thank you" comment that the code was more-or-less 
taken from/based on a2wsgi and/or that improvements to ASGIStream should be 
checked if they also apply to a2wsgi?

However, we can further discuss the changes within the PR if you want.

Kind regards,
Noxx


On Tuesday, May 17, 2022 at 7:20:18 PM UTC+2 carlton...@gmail.com wrote:

> Hi Noxx. 
>
> This is interesting, thanks for the write-up. 
>
> As a bit of history, initially ASGIRequests would read the whole body into 
> memory, which was fine for an initial version, but led quickly enough to 
> resource concerns. Andrew followed up with the SpooledTemporaryFile 
> approach, as a nice next-step improvement on that. There **was** at the 
> time some initial work on wrapping the receive queue to consume the body 
> on-demand, but with the temp-file solution in hand that never got pushed 
> through. I can very well imagine folks in ASGI-land have had time to get 
> that right now. (So exciting to look at!) 
>
> I'd very much want to dig into a dependency before deciding to take one 
> on. I suspect it's not **that** complex, so it's likely something we'd want 
> to re-implement to fit our needs exactly. (But we can see on review...) 
>
> Please do open a PR when you're ready. 
>
> Kind regards, 
> Carlton 
>
> On Tue, 17 May 2022 at 15:20, 'Noxx' via Django developers (Contributions 
> to Django itself)  wrote:
>
>> Hi all,
>>
>> I've played around a bit and would like to give you a heads up:
>>
>> WSGI and ASGI have only differences in how they parse incoming requests.
>> As WSGI is synchronous the body is already provided as a ByteIO stream 
>> which is set directly on the HTTPRequest (wrapped as a LazyStream).
>>
>> On ASGI the body is sent in chunks. As the base of HTTPRequest - and this 
>> includes how to parse the body, POST, FILES etc. - is the same for ASGI and 
>> WSGI, those chunks need to be joined together prior to any other processing.
>> to do this, all chunks are currently written into the 
>> SpooledTemporaryFile mentioned in my previous post and this file is then 
>> provided as a stream to the HTTPRequest.
>> As already written, this workaround to get all chunks into one stream is 
>> very slow for requests which are bigger in their size as the temporary file 
>> is spooled to disc in the worst case.
>>
>> To workaround this issue, I've tried to delay the reading of the ASGI 
>> queue to get the next chunk (called "message" in ASGI) until the 
>> HTTPRequest really needs it.
>> As the HTTPRequest/MultiFileParser works on a stream to read and parse 
>> its body, the idea is to use the ASGI queue as a stream-like object and to 
>> directly read it.
>> This would eliminate the need to create a temporary file as a "proxy".
>>
>> When I was in the process of implementing this interface, I've recognized 
>> that the package a2wsgi already handles such use-case:
>> https://github.com/abersheeran/a2wsgi/blob/master/a2wsgi/wsgi.py
>>
>> I've added a2wsgi as a dependency and change the ASGI handler a little 
>> bit.
>> The changes can be reviewed on my fork: 
>> https://github.com/Flauschbaellchen/django/commit/006bf2d5c0b82092524ee9b65ef919637165ee01
>>
>> Timings:
>> In my test environment, the timing went from 4s (sending)/16s (waiting) 
>> to 6s/5s.
>> Sending is a bit slower. This is because prior to the change the chunks 
>> have been read directly into the temporary file and now the parser is doing 
>> its business between reading new chunks.
>> However, those chunks are directly parsed and feed into the upload 
>> handlers which dramatically improves the performance on the server side.
>> Using gunicorn+uvicorn the waiting time went down to ~600ms. I don't know 
>> why runserver/daphne is so slow. I'll check this within the next days.
>>
>> I've attached a new flame graph representing my changes.
>> The right-most block can be ignored - this is already the next GET 
>> request of the admin panel after saving the object.
>>
>> What's next?
>> I would appreciate any feedback on my changes.
>> Is this the right direction?
>> What about the newly introduced dependency? - It would also be possible 
>> to implement/copy the code on the Django side to eliminate this dependency. 
>> However, on my opinion, doing things twice is never a good idea...
>> Can anyone test against other environments/ASGI servers? Maybe I've 
>> overlooked some edge cases.
>>
>> The current changes depends on the version 4.0.4 of Django