To clarify, I am NOT using regexp as a replacement for locations. Some of the things that I need to do might be possible with locations. Most aren’t. I have to test more than just the request_uri and logically I have to use AND/OR and nested conditions. So it’s largely unfeasible with core Nginx directives. That’s why I moved to scripting alternatives.

As for RegExp.test vs String.match, that’s true. I guess technically I should be using RegExp.test since I only care whether the value matches or not for most instances but it’s much uglier and harder to read. If it’s usable, though, that’s better than consuming loads of memory.

As a rough test, I did just convert most of the String.match references to RegExp.test and it did reduce the memory. But it only reduced it by around 40kb. That’s certainly better but on a 1mb memory footprint, not by much. Maybe that is because of what you mentioned about the internal call to RegExp.exec, though. That would make sense if it’s only mildly different from .match under the hood. So if .test is likely to diverge from .match on consumed memory over time, that’s a better fit if you need to do a lot of things that might invoke the regex engine. I’m guessing that String.replace (which I also use in a number of places) is also running its regexes through RegExp.exec too so if that’s where some of this memory leak is coming from, improvements to that might help by a lot.

For whatever it’s worth, the current math seems to play out like this:

(/someregex/).test(string)

is about 8kb smaller than:

let regex = new RegExp(/someregex/);
regex.test(string)

That makes sense since one is also creating a variable but the first route seems to be the only viable option if you need to do anything that is going to touch regex under the hood.

Thus far, the only reliable solution that I’ve found at reducing memory is to just carefully craft the order of if/then statements to ensure that no more string replacements or regex tests occur than absolutely have to for a given type of request. That’s made a significant dent in specific use cases but mostly it’s just been mild improvements. Between what you’ve shared about the regex internals and what I’ve seen doing basic tests around conditions (either using .match or .test), I do think that that is where the bulk of the problem is. Every time that I do anything that involves a regex (string replacement, etc), it seems to burn at least 20k.

Given that, it seems like I only have a few options to be able to address this at all right now and most of them are just workarounds (e.g. moving some of these regexes to maps, maybe splitting some of the logic into a few different js_set directives and localizing those to specific contexts to make them function in a bit more of a JIT way, or using an internalRedirect just for the sake of freeing memory). I’ll to try to figure out what I can. Hopefully whatever I come up with is enough to keep Nginx from spontaneously crashing during some types of concurrent traffic load. That’s my only real goal at the moment. :)

Thank you again for the feedback on all of this. It has helped to find at least a few ways to contain this even if just to improve it mildly. Every little bit seems to count for me at the moment.

—
Lance Dockins

> On Tuesday, Sep 26, 2023 at 5:51 PM, Dmitry Volyntsev <xeioex@nginx.com (mailto:xeioex@nginx.com)> wrote:
>
> On 9/26/23 8:30 AM, Lance Dockins wrote:
> > Up until now, I had assumed that string.match types of statements were
> > just transparently calling PCRE behind the scenes so that the
> > associated memory from the PCRE call was being freed after use. Maybe
> > that’s not even an accurate read on how Nginx is using PCRE but that’s
> > how I envisioned it.
>
> String.prototype.match() returns an array of matched elements. See
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match.
> It means that many calls to that method that have positive matches
> consume memory proportional to the size of the resulting chunks. But if
> the match fails it should not consume (much) memory. So if you have a
> bunch of if/then clauses until the first match it should not be a
> problem (if not let me know).
>
>
> In general doing a routing in NJS (or any other scripting) is not
> recommended, because it complicates the native nginx location routing.
> Could you please explain what exactly you want to do with NJS RegExp
> what nginx cannot do with regexp locations?
>
> >
> > In any case, that lead me to a few questions.
> >
> > 1. How does keepalive affect these VMs? I realize that keepalive is
> > for the connection rather than the request, but I still wanted to
> > confirm that VM’s are not sticking around occupying memory after
> > the request completes due to keepavlive settings.
> >
> >
> All the current VMs are destroyed when a current HTTP request is finalized.
>
>
> > 1. We have a mix of fast requests and long running requests (e.g
> > 5-10s) so if enough long running requests build up at the same
> > time that a flood of very fast requests come in, NJS could
> > certainly burn out a lot of memory under the current conditions as
> > I understand it. If there were a lot of large file downloads,
> > those would also occupy memory for the entirety of the download if
> > I understand correctly.
> > 2. Is it possible to adjust the NJS codebase to free memory for the
> > actual if condition and for string matches after they’ve run as
> > long as they didn’t include a variable assignment inside of the
> > actual if condition? I’m not talking about whatever is in the
> > block content - just the actual condition. Given how likely it is
> > for people to use basic if conditions and string matches in NJS,
> > that seems like it might be a stopgap measure that reduces the
> > memory footprint without having to build full garbage collection.
> >
> >
> The problem with JS Regexps is that they produce a lot of intermediary
> objects. Because most of the RegExp related calls end up
> RegExp.prototype.exec() which return a large object representing the
> result of PCRE matching.
>
>
> One way to mediate the problem could be using RegExp.prototype.test()
> which return just a boolean.
> But I need to improve it first, because right now it uses
> RegExp.prototype.exec() internally. The reason RegExp.prototype.test()
> will be easier to improve is that I can be sure that the resulting
> object can be freed right away. In addition to that I plan to improve
> the memory consumption for RegExp.prototype.exec() result, thanks for
> reporting.
>
>
> >
> > 1. Is there any workaround for this type of memory problem other than
> > just to force the use of an internalRedirect to drop the existing
> > VM and create a new one?
> >
> > 1. Could it make sense to allow for a directive that would force the
> > destruction and regeneration of a new JS VM? That wouldn’t solve
> > the memory leak that bulids to 1mb per request but it would
> > shorten its lifetime (which could ease memory pressure in
> > situations where there are some long running requests holding open
> > the requests)
> >
> I do not think so.
>
>
_______________________________________________
nginx mailing list
nginx@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx