[njs] Improved String.prototype.replaceAll() for readability.
details: https://hg.nginx.org/njs/rev/077a5b2f30d8
branches:
changeset: 2336:077a5b2f30d8
user: Dmitry Volyntsev
date: Wed May 22 17:26:16 2024 -0700
description:
Improved String.prototype.replaceAll() for readability.
diffstat:
src/njs_string.c | 44 ++--
1 files changed, 26 insertions(+), 18 deletions(-)
diffs (100 lines):
diff -r e496851c0fe7 -r 077a5b2f30d8 src/njs_string.c
--- a/src/njs_string.c Wed May 22 17:26:08 2024 -0700
+++ b/src/njs_string.c Wed May 22 17:26:16 2024 -0700
@@ -3228,7 +3228,7 @@ njs_string_prototype_replace(njs_vm_t *v
njs_index_t replaceAll, njs_value_t *retval)
{
u_char *r;
-size_t length, size, increment, end_of_last_match;
+size_t length, size, end_of_last_match;
int64_tpos;
njs_int_t ret;
njs_str_t str;
@@ -3236,7 +3236,7 @@ njs_string_prototype_replace(njs_vm_t *v
njs_value_t*this, *search, *replace;
njs_value_tsearch_lvalue, replace_lvalue, replacer, value,
arguments[3];
-const u_char *p, *p_start;
+const u_char *start, *end;
njs_function_t *func_replace;
njs_string_prop_t string, s, ret_string;
@@ -3331,7 +3331,6 @@ njs_string_prototype_replace(njs_vm_t *v
}
if (!replaceAll) {
-
if (func_replace == NULL) {
ret = njs_string_get_substitution(vm, search, this, pos, NULL, 0,
NULL, replace, );
@@ -3356,7 +3355,7 @@ njs_string_prototype_replace(njs_vm_t *v
}
}
-p = njs_string_offset(, pos);
+end = njs_string_offset(, pos);
(void) njs_string_prop(_string, );
@@ -3368,17 +3367,16 @@ njs_string_prototype_replace(njs_vm_t *v
return NJS_ERROR;
}
-r = njs_cpymem(r, string.start, p - string.start);
+r = njs_cpymem(r, string.start, end - string.start);
r = njs_cpymem(r, ret_string.start, ret_string.size);
-memcpy(r, p + s.size, string.size - s.size - (p - string.start));
+memcpy(r, end + s.size, string.size - s.size - (end - string.start));
return NJS_OK;
}
NJS_CHB_MP_INIT(, vm);
-p_start = string.start;
-increment = s.length != 0 ? s.length : 1;
+start = string.start;
do {
if (func_replace == NULL) {
@@ -3405,20 +3403,30 @@ njs_string_prototype_replace(njs_vm_t *v
}
}
-p = njs_string_offset(, pos);
+end = njs_string_offset(, pos);
(void) njs_string_prop(_string, );
-njs_chb_append(, p_start, p - p_start);
+njs_chb_append(, start, end - start);
njs_chb_append(, ret_string.start, ret_string.size);
-p_start = p + s.size;
-end_of_last_match = pos + increment;
-
-pos = njs_string_index_of(, , end_of_last_match);
-
-} while (pos >= 0 && end_of_last_match <= string.length);
-
-njs_chb_append(, p_start, string.start + string.size - p_start);
+start = end + s.size;
+end_of_last_match = pos + s.length;
+
+if (njs_slow_path(s.length == 0)) {
+if (end_of_last_match >= string.length) {
+pos = -1;
+
+} else {
+pos = end_of_last_match + 1;
+}
+
+} else {
+pos = njs_string_index_of(, , end_of_last_match);
+}
+
+} while (pos >= 0);
+
+njs_chb_append(, start, string.start + string.size - start);
ret = njs_string_create_chb(vm, retval, );
if (njs_slow_path(ret != NJS_OK)) {
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[njs] Fixed String.prototype.replaceAll() with zero length argument.
details: https://hg.nginx.org/njs/rev/e496851c0fe7
branches:
changeset: 2335:e496851c0fe7
user: Dmitry Volyntsev
date: Wed May 22 17:26:08 2024 -0700
description:
Fixed String.prototype.replaceAll() with zero length argument.
This fixes #712 issue on Github.
diffstat:
src/njs_string.c | 15 ---
src/njs_string.h | 4
src/test/njs_unit_test.c | 6 ++
3 files changed, 14 insertions(+), 11 deletions(-)
diffs (76 lines):
diff -r bc09b884022d -r e496851c0fe7 src/njs_string.c
--- a/src/njs_string.c Tue May 21 23:41:10 2024 -0700
+++ b/src/njs_string.c Wed May 22 17:26:08 2024 -0700
@@ -1736,7 +1736,9 @@ njs_string_index_of(njs_string_prop_t *s
} else {
/* UTF-8 string. */
-p = njs_string_utf8_offset(string->start, end, index);
+p = (index < string->length)
+? njs_string_utf8_offset(string->start, end, index)
+: end;
end -= search->size - 1;
while (p < end) {
@@ -3231,7 +3233,6 @@ njs_string_prototype_replace(njs_vm_t *v
njs_int_t ret;
njs_str_t str;
njs_chb_t chain;
-njs_bool_t is_ascii_string;
njs_value_t*this, *search, *replace;
njs_value_tsearch_lvalue, replace_lvalue, replacer, value,
arguments[3];
@@ -3378,7 +3379,6 @@ njs_string_prototype_replace(njs_vm_t *v
p_start = string.start;
increment = s.length != 0 ? s.length : 1;
-is_ascii_string = njs_is_ascii_string();
do {
if (func_replace == NULL) {
@@ -3405,14 +3405,7 @@ njs_string_prototype_replace(njs_vm_t *v
}
}
-if (is_ascii_string) {
-p = string.start + pos;
-
-} else {
-p = njs_string_utf8_offset(string.start, string.start +
string.size,
- pos);
-}
-
+p = njs_string_offset(, pos);
(void) njs_string_prop(_string, );
njs_chb_append(, p_start, p - p_start);
diff -r bc09b884022d -r e496851c0fe7 src/njs_string.h
--- a/src/njs_string.h Tue May 21 23:41:10 2024 -0700
+++ b/src/njs_string.h Wed May 22 17:26:08 2024 -0700
@@ -245,6 +245,10 @@ njs_string_offset(njs_string_prop_t *str
/* UTF-8 string. */
+if (index == (int64_t) string->length) {
+return string->start + string->size;
+}
+
return njs_string_utf8_offset(string->start, string->start + string->size,
index);
}
diff -r bc09b884022d -r e496851c0fe7 src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c Tue May 21 23:41:10 2024 -0700
+++ b/src/test/njs_unit_test.c Wed May 22 17:26:08 2024 -0700
@@ -,6 +,12 @@ static njs_unit_test_t njs_test[] =
{ njs_str("var r = 'αβγ'.replaceAll('', 'X'); [r, r.length]"),
njs_str("XαXβXγX,7") },
+{ njs_str("var r = ''.replaceAll('', 'z'); [r, r.length]"),
+ njs_str("z,1") },
+
+{ njs_str("var r = 'α'.padStart(32).replaceAll('', 'z'); [r, r.length]"),
+ njs_str("z z z z z z z z z z z z z z z z z z z z z z z z z z z z z z z
zαz,65") },
+
{ njs_str("'abc'.replace('b', (m, o, s) => `|${s}|${o}|${m}|`)"),
njs_str("a|abc|1|b|c") },
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[njs] Aligned StringIndexOf() implementation with the spec.
details: https://hg.nginx.org/njs/rev/bc09b884022d
branches:
changeset: 2334:bc09b884022d
user: Dmitry Volyntsev
date: Tue May 21 23:41:10 2024 -0700
description:
Aligned StringIndexOf() implementation with the spec.
When searchValue is empty the function should return early
when fromIndex <= len is also true.
diffstat:
src/njs_string.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diffs (14 lines):
diff -r 2d098d2a1c85 -r bc09b884022d src/njs_string.c
--- a/src/njs_string.c Tue May 21 23:41:10 2024 -0700
+++ b/src/njs_string.c Tue May 21 23:41:10 2024 -0700
@@ -1710,8 +1710,8 @@ njs_string_index_of(njs_string_prop_t *s
length = string->length;
-if (njs_slow_path(search->length == 0)) {
-return (from < length) ? from : length;
+if (search->length == 0 && from <= length) {
+return from;
}
index = from;
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[njs] Removed code for byte strings in built-in functions.
details: https://hg.nginx.org/njs/rev/2d098d2a1c85
branches:
changeset: 2333:2d098d2a1c85
user: Dmitry Volyntsev
date: Tue May 21 23:41:10 2024 -0700
description:
Removed code for byte strings in built-in functions.
diffstat:
src/njs_array.c | 52 --
src/njs_iterator.c | 2 +-
src/njs_json.c | 9 +-
src/njs_object.c | 4 +-
src/njs_regexp.c | 16 +++
src/njs_string.c | 65 +--
src/njs_string.h | 20 ++---
src/njs_vmcode.c | 9 --
src/test/njs_unit_test.c | 30 +-
9 files changed, 49 insertions(+), 158 deletions(-)
diffs (596 lines):
diff -r 27da19960b72 -r 2d098d2a1c85 src/njs_array.c
--- a/src/njs_array.c Tue May 21 23:38:19 2024 -0700
+++ b/src/njs_array.c Tue May 21 23:41:10 2024 -0700
@@ -826,29 +826,15 @@ njs_array_prototype_slice_copy(njs_vm_t
src = string.start;
end = src + string.size;
-if (string.length == 0) {
-/* Byte string. */
-do {
-value = >start[n++];
-dst = njs_string_short_start(value);
-*dst = *src++;
-njs_string_short_set(value, 1, 0);
-
-length--;
-} while (length != 0);
-
-} else {
-/* UTF-8 or ASCII string. */
-do {
-value = >start[n++];
-dst = njs_string_short_start(value);
-dst = njs_utf8_copy(dst, , end);
-size = dst - njs_string_short_start(value);
-njs_string_short_set(value, size, 1);
-
-length--;
-} while (length != 0);
-}
+do {
+value = >start[n++];
+dst = njs_string_short_start(value);
+dst = njs_utf8_copy(dst, , end);
+size = dst - njs_string_short_start(value);
+njs_string_short_set(value, size, 1);
+
+length--;
+} while (length != 0);
} else if (njs_is_object(this)) {
@@ -1647,11 +1633,10 @@ static njs_int_t
njs_array_prototype_join(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
njs_index_t unused, njs_value_t *retval)
{
-u_char *p, *last;
+u_char *p;
int64_ti, size, len, length;
njs_int_t ret;
njs_chb_t chain;
-njs_utf8_t utf8;
njs_value_t*value, *this, entry;
njs_string_prop_t separator, string;
@@ -1684,7 +1669,6 @@ njs_array_prototype_join(njs_vm_t *vm, n
}
length = 0;
-utf8 = njs_is_byte_string() ? NJS_STRING_BYTE : NJS_STRING_UTF8;
ret = njs_object_length(vm, this, );
if (njs_slow_path(ret == NJS_ERROR)) {
@@ -1708,29 +1692,15 @@ njs_array_prototype_join(njs_vm_t *vm, n
if (!njs_is_null_or_undefined(value)) {
if (!njs_is_string(value)) {
-last = njs_chb_current();
-
ret = njs_value_to_chain(vm, , value);
if (njs_slow_path(ret < NJS_OK)) {
return ret;
}
-if (last != njs_chb_current() && ret == 0) {
-/*
- * Appended values was a byte string.
- */
-utf8 = NJS_STRING_BYTE;
-}
-
length += ret;
} else {
(void) njs_string_prop(, value);
-
-if (njs_is_byte_string()) {
-utf8 = NJS_STRING_BYTE;
-}
-
length += string.length;
njs_chb_append(, string.start, string.size);
}
@@ -1755,7 +1725,7 @@ njs_array_prototype_join(njs_vm_t *vm, n
length -= separator.length;
-p = njs_string_alloc(vm, retval, size, utf8 ? length : 0);
+p = njs_string_alloc(vm, retval, size, length);
if (njs_slow_path(p == NULL)) {
return NJS_ERROR;
}
diff -r 27da19960b72 -r 2d098d2a1c85 src/njs_iterator.c
--- a/src/njs_iterator.cTue May 21 23:38:19 2024 -0700
+++ b/src/njs_iterator.cTue May 21 23:41:10 2024 -0700
@@ -355,7 +355,7 @@ njs_object_iterate(njs_vm_t *vm, njs_ite
end = p + string_prop.size;
if ((size_t) length == string_prop.size) {
-/* Byte or ASCII string. */
+/* ASCII string. */
for (i = from; i < to; i++) {
/* This cannot fail. */
diff -r 27da19960b72 -r 2d098d2a1c85 src/njs_json.c
--- a/src/njs_json.cTue May 21 23:38:19 2024 -0700
+++ b/src/njs_json.cTue May 21 23:41:10 2024 -0700
@@ -226,13 +226,6 @@ njs_json_stringify(njs_vm_t *vm, njs_val
switch (space->type) {
case NJS_STRING:
length =
[njs] Avoid creating byte strings explicitly.
details: https://hg.nginx.org/njs/rev/27da19960b72
branches:
changeset: 2332:27da19960b72
user: Dmitry Volyntsev
date: Tue May 21 23:38:19 2024 -0700
description:
Avoid creating byte strings explicitly.
Previously, calls like njs_string_new(,,,0) produced byte strings by
explicitly providing zero length argument.
diffstat:
src/njs_builtin.c | 4 ++--
src/njs_json.c| 8 +---
src/njs_vm.c | 2 +-
3 files changed, 4 insertions(+), 10 deletions(-)
diffs (58 lines):
diff -r 286dbef3c0b2 -r 27da19960b72 src/njs_builtin.c
--- a/src/njs_builtin.c Mon May 20 16:44:10 2024 -0700
+++ b/src/njs_builtin.c Tue May 21 23:38:19 2024 -0700
@@ -388,7 +388,7 @@ njs_builtin_traverse(njs_vm_t *vm, njs_t
return NJS_ERROR;
}
-ret = njs_string_new(vm, >name, buf, p - buf, 0);
+ret = njs_string_create(vm, >name, buf, p - buf);
if (njs_slow_path(ret != NJS_OK)) {
return ret;
}
@@ -590,7 +590,7 @@ njs_ext_dump(njs_vm_t *vm, njs_value_t *
return NJS_ERROR;
}
-return njs_string_new(vm, retval, str.start, str.length, 0);
+return njs_string_create(vm, retval, str.start, str.length);
}
diff -r 286dbef3c0b2 -r 27da19960b72 src/njs_json.c
--- a/src/njs_json.cMon May 20 16:44:10 2024 -0700
+++ b/src/njs_json.cTue May 21 23:38:19 2024 -0700
@@ -548,7 +548,6 @@ njs_json_parse_string(njs_json_parse_ctx
{
u_charch, *s, *dst;
size_tsize, surplus;
-ssize_t length;
uint32_t utf, utf_low;
njs_int_t ret;
const u_char *start, *last;
@@ -742,12 +741,7 @@ njs_json_parse_string(njs_json_parse_ctx
start = dst;
}
-length = njs_utf8_length(start, size);
-if (njs_slow_path(length < 0)) {
-length = 0;
-}
-
-ret = njs_string_new(ctx->vm, value, (u_char *) start, size, length);
+ret = njs_string_create(ctx->vm, value, (u_char *) start, size);
if (njs_slow_path(ret != NJS_OK)) {
return NULL;
}
diff -r 286dbef3c0b2 -r 27da19960b72 src/njs_vm.c
--- a/src/njs_vm.c Mon May 20 16:44:10 2024 -0700
+++ b/src/njs_vm.c Tue May 21 23:38:19 2024 -0700
@@ -874,7 +874,7 @@ njs_vm_bind2(njs_vm_t *vm, const njs_str
njs_lvlhsh_t*hash;
njs_lvlhsh_query_t lhq;
-ret = njs_string_new(vm, >name, var_name->start, var_name->length,
0);
+ret = njs_string_create(vm, >name, var_name->start,
var_name->length);
if (njs_slow_path(ret != NJS_OK)) {
return NJS_ERROR;
}
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: I think I found a fix for the memory leak issue on gRPC module
Hi,
Indeed there's a problem there. We have similar problems in other places as
well. Attached is a patch that fixes all I could find.
I did some testing for the sub_filter with the following config. Small buffers
exaggerate the problem.
http {
server {
listen 8000;
location / {
root html;
output_buffers 2 128;
sub_filter 1 2;
sub_filter_types *;
sub_filter_once off;
}
}
}
Retrieving a 10m file resulted in 7304 (patched) vs 1317704 (unpatched) bytes
allocated from the request pool. With the default output_buffers (2 32768),
it's 79880 vs 84040.
On Thu, May 02, 2024 at 07:59:44AM +, Pavel Pautov via nginx-devel wrote:
> Hi Sangmin,
>
> Your analysis looks correct. Client streaming RPCs can lead to unbound
> accumulation of ngx_chain_t objects (although, at very slow rate). Gzip
> module had a similar issue (https://trac.nginx.org/nginx/ticket/1046).
> Attaching somewhat simplified patch based on yours. I was able to reproduce
> the issue locally and the patch fixes it.
>
> From: nginx-devel On Behalf Of Sangmin Lee
> Sent: Thursday, April 4, 2024 19:29
> To: nginx-devel@nginx.org
> Subject: I think I found a fix for the memory leak issue on gRPC module
>
> CAUTION: This email has been sent from an external source. Do not click
> links, open attachments, or provide sensitive business information unless you
> can verify the sender's legitimacy.
>
> I am sending this mail again because I did a mistake while I was writing a
> mail. I didn't know, in gmail, "Ctrl - Enter" would send a mail immediately
> even without a title!
> I am sorry for that, so I am sending again.
>
> Hello,
>
> I think I found the main cause of the memory leak issue when using gRPC
> stream so I made a patch for it.
> Please find the test scenario and details here -- This is what I wrote.:
> https://trac.nginx.org/nginx/ticket/2614
> After I changed the memory pool totally on nginx and tested our workload --
> long-lived gRPC streams with many connections -- using Valgrind and massif, I
> was able to find what brought up the memory leak issue.
> like the picture below.
>
> [cid:image001.png@01DA9C29.C792CD90]
> ( I am not sure whether this picture will be sent properly )
>
> After I patched one part, it seems okay now I have tested it for 1 week with
> out workload.
>
> [cid:image002.png@01DA9C29.C792CD90]
> ( I am not sure whether this picture will be sent properly )
>
> But because I am not familiar with Mercurial, I couldn't find a way to create
> PR like on github. I guess this mailing list is for this patch.
> From my point of view, it is more like a workaround and I think the way of
> using ngx_chain_add_copy() or itself needs to be changed because it allocates
> a ngx_chain_t structure using ngx_alloc_chain_link() but inside of that, it
> just copies pointer, like cl->buf = in->buf;
> so this ngx_chain_t instance should be dealt with differently unlike other
> ngx_chain_t instances.
> But I am quite new to nginx codes so my view might be wrong.
> Anyhow, please go over this patch and I would like to further talk here.
>
>
>
> diff --git a/src/http/modules/ngx_http_grpc_module.c
> b/src/http/modules/ngx_http_grpc_module.c
> index dfe49c586..1db67bd0a 100644
> --- a/src/http/modules/ngx_http_grpc_module.c
> +++ b/src/http/modules/ngx_http_grpc_module.c
> @@ -1462,6 +1462,12 @@ ngx_http_grpc_body_output_filter(void *data,
> ngx_chain_t *in)
> in = in->next;
> }
>
> + ngx_chain_t *nl;
> + for (ngx_chain_t *dl = ctx->in; dl != in; dl = nl ) {
> + nl = dl->next;
> + ngx_free_chain(r->pool, dl);
> + }
> +
> ctx->in = in;
>
> if (last) {
>
>
>
> Best regards,
> Sangmin
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan
# Date 1716386385 -14400
# Wed May 22 17:59:45 2024 +0400
# Node ID 95af5fe4921294b48e634defc03b6b0f0201d6f7
# Parent e60377bdee3d0f8dbd237b5ae8a5deab2af785ef
Optimized chain link usage (ticket #2614).
Previously chain links could sometimes be dropped instead of being reused,
which could result in increased memory consumption during long requests.
A similar chain link issue in ngx_http_gzip_filter_module was fixed in
da46bfc484ef (1.11.10).
Based on a patch by Sangmin Lee.
diff --git a/src/core/ngx_output_chain.c b/src/core/ngx_output_chain.c
--- a/src/core/ngx_output_chain.c
+++ b/src/core/ngx_output_chain.c
@@ -117,7 +117,10 @@
