Hello! On Tue, Oct 17, 2023 at 03:25:41PM +0400, Sergey Kandaurov wrote:
> > On 11 Oct 2023, at 02:56, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > Hello! > > > > On Thu, Oct 05, 2023 at 10:51:26AM +0900, Yusuke Nojima wrote: > > > >> Thank you for your comment! > >> > >>> Could you please provide some more details about the use case, > >>> such as how locations are used, and what is the approximate number > >>> of locations being used? > >> > >> Our team provides development environments to our company's engineers and > >> QA. > >> In this environment, engineers and QA can freely create VMs and deploy > >> applications on them. > >> > >> Our nginx has the role of routing requests from the internet to all > >> applications deployed in this environment. > >> Additionally, it allows setting IP address restrictions, BASIC > >> authentication, TLS client authentication, and other configurations > >> for each application. > >> > >> To implement these requirements, we generate a location for each > >> application. > >> Currently, there are approximately 40,000 locations in our environment. > > > > Thank you for the details. Such configuration looks somewhat > > sub-optimal, but understandable for a development / test > > environment. And certainly 40k locations is a lot for the sorting > > algorithm currently used. > > > >>> Further, since each location contains configuration for > >>> all modules, such configurations are expected to require a lot of > >>> memory > >> > >> Each of our nginx processes was consuming 5GB of memory in terms of > >> resident size. > >> This is not a problem as our servers have sufficient memory. > >> > >>> Rather, I would suggest recursive top-bottom merge sort implementation > >>> instead, which is much simpler and uses stack as temporary storage > >>> (so it'll naturally die if there will be a queue which requires > >>> more space for sorting than we have). > > For the record, in my tests on M1 sorting 26^3 locations fit into > 32k stack size (16k stack size renders the environment unusable). > Judging by this (unscientific) test, running out of stack should > not be a practicable issue. The recursive function uses ngx_queue_t *q, tail; on stack, that is, 3 pointers, so it would need more than 1000 recursive calls to overflow 32k stack, which is not expected to happen unless there are more than 2^1000 locations (and it is certainly not possible to have more than 2^64 locations on 64-bit platforms). For 26^3 locations, sorting would use maximum recursion depth of 15, so would require something like 360 bytes of stack (which is certainly less than required by other nginx functions). > >>> > >>> Please take a look if it works for you: > >> > >> I think this implementation is simple and easy to understand. > >> Although the number of traversals of the list will increase compared > >> to bottom-up, it will not affect the order. > >> I believe this will provide sufficient optimization in terms of speed. > > > > Thanks for looking. In my limited testing, it is slightly faster > > than your bottom-up implementation (and significantly faster than > > the existing insertion sort when many locations are used). > > > > Below is the full patch (code unchanged), I'll commit it as soon > > as some other nginx developer will review it. > > > > # HG changeset patch > > # User Maxim Dounin <mdou...@mdounin.ru> > > # Date 1696977468 -10800 > > # Wed Oct 11 01:37:48 2023 +0300 > > # Node ID b891840852ee5cc823eee1769d092ab50928919f > > # Parent cdda286c0f1b4b10f30d4eb6a63fefb9b8708ecc > > Core: changed ngx_queue_sort() to use merge sort. > > > > This improves nginx startup times significantly when using very large number > > of locations due computational complexity of the sorting algorithm being > > due to Fixed, thnx. > > used (insertion sort is O(n*n) on average, while merge sort is O(n*log(n))). > > nitpicking: E2MANYPARENS > > Using a colon might looks better (I don't mind though): > used: insertion sort is O(n*n) on average, while merge sort is O(n*log(n)). Thanks, changed. > > In particular, in a test configuration with 20k locations total startup > > time is reduced from 8 seconds to 0.9 seconds. > > > > Prodded by Yusuke Nojima, > > https://mailman.nginx.org/pipermail/nginx-devel/2023-September/NUL3Y2FPPFSHMPTFTL65KXSXNTX3NQMK.html > > I like the change, please commit. > > The thing to keep in mind is that it pessimizes the best case > of sorted locations, which is O(n) with the insertion sort. > Though given that both old and new algorithms give relatively > good numbers for the best case (and it is hard to get a > noticeable startup delay with very large number of locations > in practice using merge sort), especially compared to the worst > case of sorting perfectly reverse sorted locations with the > insertion sort, I believe this is acceptable. Sure, theoretically insertion sort's best case is slightly better, but I don't think it matters: if at all, the difference is negligible and unlikely can be measured, while noticeable startup delays with large number of non-sorted locations are easily observed with insertion sort. For the record, here are "nginx -t" times with 20k pre-sorted locations, with insertion sort (before the patch): 0.64 real 0.49 user 0.14 sys 0.59 real 0.51 user 0.07 sys 0.61 real 0.48 user 0.12 sys 0.60 real 0.49 user 0.11 sys 0.61 real 0.51 user 0.09 sys And with merge sort (after the patch): 0.65 real 0.50 user 0.14 sys 0.63 real 0.49 user 0.14 sys 0.63 real 0.52 user 0.10 sys 0.62 real 0.49 user 0.12 sys 0.67 real 0.55 user 0.11 sys And ministat(1) for the user times: $ ministat insert merge x insert + merge +------------------------------------------------------------------------------+ | * x | |x * + x + +| | |_|_____M______A___M_________|A___________________________| | +------------------------------------------------------------------------------+ N Min Max Median Avg Stddev x 5 0.48 0.51 0.49 0.496 0.013416408 + 5 0.49 0.55 0.5 0.51 0.025495098 No difference proven at 95.0% confidence Thanks for looking, pushed to http://mdounin.ru/hg/nginx/ -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel