Opened 14 months ago

Closed 14 months ago

Last modified 4 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 Changed 14 months ago by vbart

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 Changed 14 months ago by mdounin

  • Resolution set to wontfix
  • Status changed from new to closed

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 Changed 10 months ago by mdounin

See also #1658.

comment:4 Changed 4 months ago by mdounin

See also #1779.

Note: See TracTickets for help on using tickets.