Opened 2 years ago

Closed 2 years ago

Last modified 15 months ago

#1594 closed defect (wontfix)

ngx_palloc.c may save more memory

Reported by: lwppwl@… Owned by:
Priority: minor Milestone:
Component: other Version: 1.12.x
Keywords: Cc:
uname -a: Linux lwp-All-Series 4.15.0-24-generic #26~16.04.1-Ubuntu SMP Fri Jun 15 14:35:08 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.12.2
built by gcc 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.9)
configure arguments: --prefix=/data/modules/nginx

Description

in nginx-1.12.2 version

the ngx_reset_pool() function in ngx_palloc.c may change better:

when reset the point of the p->d.last, should consider the p whether or not the pool,
when the p is not the pool, should offset sizeof(ngx_pool_data_t) not sizeof(ngx_pool_t),

every small block can save sizeof(ngx_pool_t) - sizeof(ngx_pool_data_t) bytes.


Original code:

void ngx_reset_pool(ngx_pool_t *pool)
{
    ...
    for (p = pool; p; p = p->d.next)
    {
        p->d.last = (u_char *) p + sizeof(ngx_pool_t);
        p->d.failed = 0;
    }
    ...
}

Suggested code:

void ngx_reset_pool(ngx_pool_t *pool)
{
    ...
    for (p = pool; p; p = p->d.next)
    {
        if (p == pool)
        {
            p->d.last = (u_char *) p + sizeof(ngx_pool_t);
            p->d.failed = 0;
        }
        else
        {
            p->d.last = (u_char *) p + sizeof(ngx_pool_data_t);
            p->d.failed = 0;
        }
    }
    ...
}

Change History (4)

comment:1 by Valentin V. Bartenev, 2 years ago

The suggested code looks very strange. You don't need this branching inside the "for" loop. You don't need it outside the loop either.

Here's an equivalent code:

  • src/core/ngx_palloc.c

    diff -r 118885f7a577 src/core/ngx_palloc.c
    a b ngx_reset_pool(ngx_pool_t *pool)  
    108108        }
    109109    }
    110110
    111     for (p = pool; p; p = p->d.next) {
    112         p->d.last = (u_char *) p + sizeof(ngx_pool_t);
     111    pool->d.last = (u_char *) pool + sizeof(ngx_pool_t);
     112    pool->d.failed = 0;
     113
     114    for (p = pool->d.next; p; p = p->d.next) {
     115        p->d.last = (u_char *) p + sizeof(ngx_pool_data_t);
    113116        p->d.failed = 0;
    114117    }
    115118

comment:2 by Maxim Dounin, 2 years ago

Resolution: wontfix
Status: newclosed

Quoting http://mailman.nginx.org/pipermail/nginx/2016-July/051280.html:

A previous attempt to "fix" this can be found here, it looks
slightly better from my point of view:

http://mailman.nginx.org/pipermail/nginx-devel/2010-June/000351.html

Though we are quite happy with the current code, while it is not
optimal - it is simple and good enough from practical point of
view.

I don't think anything changed since then.

comment:3 by Maxim Dounin, 22 months ago

See also #1658.

comment:4 by Maxim Dounin, 15 months ago

See also #1779.

Note: See TracTickets for help on using tickets.