Opened 5 years ago

Closed 5 years ago

#1721 closed enhancement (fixed)

One line redundant code in ngx_slab_free_locked

Reported by: chronolaw@… Owned by:
Priority: minor Milestone:
Component: nginx-core Version: 1.15.x
Keywords: ngx_slab_free Cc:
uname -a:
nginx -V: nginx version: nginx/1.15.8

Description

Hi, When I read the source code of Nginx, I notice there is one line redundant code in ngx_slab_free_locked.

n = ((u_char *) p - pool->start) >> ngx_pagesize_shift;

In the beginning of this function, 'n' has been calculated, but in the case NGX_SLAB_PAGE, it is calculated again.

Please see the patch, thanks.

Attachments (1)

ngx_slab.c.patch (695 bytes ) - added by chronolaw@… 5 years ago.

Download all attachments as: .zip

Change History (5)

by chronolaw@…, 5 years ago

Attachment: ngx_slab.c.patch added

comment:1 by Maxim Dounin, 5 years ago

The n variable is used in the following &pool->pages[n] reference, and using generic multi-purpose variables in the parts of code distant from the code where it is calculated is generally a bad idea. Even if in a particular moment the variable stays intact, it can be overwritten at some point later, breaking things.

On the other hand, calculating &pool->pages[n] looks unneeded too, as we already have page pointer. It should be fine to simply use page instead of re-calculating it. Please test if the following patch works for you:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1549854991 -10800
#      Mon Feb 11 06:16:31 2019 +0300
# Node ID aa5fa9166b0b81f084f64b578707ec807d03cab1
# Parent  1aa074869d804fe822add52dbaeda978fc749cf6
Slab: removed redundant page calculation (ticket #1721).

diff --git a/src/core/ngx_slab.c b/src/core/ngx_slab.c
--- a/src/core/ngx_slab.c
+++ b/src/core/ngx_slab.c
@@ -635,10 +635,9 @@ ngx_slab_free_locked(ngx_slab_pool_t *po
             goto fail;
         }
 
-        n = ((u_char *) p - pool->start) >> ngx_pagesize_shift;
         size = slab & ~NGX_SLAB_PAGE_START;
 
-        ngx_slab_free_pages(pool, &pool->pages[n], size);
+        ngx_slab_free_pages(pool, page, size);
 
         ngx_slab_junk(p, size << ngx_pagesize_shift);
 

comment:2 by chronolaw@…, 5 years ago

I think this is a better patch, thanks.

comment:3 by Maxim Dounin <mdounin@…>, 5 years ago

In 7457:d97d09ef3afe/nginx:

Slab: removed redundant page calculation (ticket #1721).

comment:4 by Maxim Dounin, 5 years ago

Resolution: fixed
Status: newclosed

Committed, thanks.

Note: See TracTickets for help on using tickets.