618790 - handling of chunk in arena_run_alloc while loop is odd

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[timeless]

3190 static arena_run_t *
3191 arena_run_alloc(arena_t *arena, arena_bin_t *bin, size_t size, bool large,
3192     bool zero)
3193 {
3194         arena_chunk_t *chunk;
3195         arena_run_t *run;
3196         arena_chunk_map_t *mapelm, key;
3197 
3198         assert(size <= arena_maxclass);
3199         assert((size & pagesize_mask) == 0);
3200 

assignment of null to chunk:
3201         chunk = NULL;

this loop is only run once: <- this seems to not be expected by the code:
3202         while (true) {
3203                 /* Search the arena's chunks for the lowest best fit. */
3204                 key.bits = size | CHUNK_MAP_KEY;
3205                 mapelm = arena_avail_tree_nsearch(&arena->runs_avail, &key);
3206                 if (mapelm != NULL) {

assignment to run_chunk (not chunk):
3207                         arena_chunk_t *run_chunk =
3208                             (arena_chunk_t*)CHUNK_ADDR2BASE(mapelm);
3209                         size_t pageind = ((uintptr_t)mapelm -
3210                             (uintptr_t)run_chunk->map) /
3211                             sizeof(arena_chunk_map_t);
3212 

chunk has not be changed, and is thus null, this statement will never be true:
3213                         if (chunk != NULL)
this statement will never be reached:
3214                                 chunk_dealloc(chunk, chunksize);
3215                         run = (arena_run_t *)((uintptr_t)run_chunk + (pageind
3216                             << pagesize_2pow));
3217                         arena_run_split(arena, run, size, large, zero);

this if condition block always exits through here - RETURN:
3218                         return (run);
3219                 }
3220 
3221                 if (arena->spare != NULL) {
3222                         /* Use the spare. */

assignment to chunk:
3223                         chunk = arena->spare;
3224                         arena->spare = NULL;
3225                         run = (arena_run_t *)((uintptr_t)chunk +
3226                             (arena_chunk_header_npages << pagesize_2pow));
3227                         /* Insert the run into the runs_avail tree. */
3228                         arena_avail_tree_insert(&arena->runs_avail,
3229                             &chunk->map[arena_chunk_header_npages]);
3230                         arena_run_split(arena, run, size, large, zero);

this if condition block always exits through here - RETURN:
3231                         return (run);
3232                 }
3233 
3234                 /*
3235                  * No usable runs.  Create a new chunk from which to allocate
3236                  * the run.
3237                  */

chunk has not be changed, and is thus null, this statement will never be false:
3238                 if (chunk == NULL) {
3239                         chunk = (arena_chunk_t *)chunk_alloc(chunksize, true,
3240                             true);
3241                         if (chunk == NULL)
this if condition always exits through here - RETURN:
3242                                 return (NULL);

this if condition block either exits through the previous RETURN or just continues:
3243                 }
3244 
3245                 arena_chunk_init(arena, chunk);
3246                 run = (arena_run_t *)((uintptr_t)chunk +
3247                     (arena_chunk_header_npages << pagesize_2pow));
3248                 /* Update page map. */
3249                 arena_run_split(arena, run, size, large, zero);

this while loop either exits through the previous RETURNs or exits through here - RETURN:
3250                 return (run);

this statement (the way to loop) is never reached:
3251         }

the lack of a return statement here is correct because the "while" loop never exit:
3252 }

Comment 1

[timeless]

Attachment: proposal a - remove loop

this assumes the loop doesn't belong. i'm on os x which doesn't build jemalloc, so i don't know if it builds

Comment 2

[Phil Ringnalda (:philor)]

http://tbpl.mozilla.org/?tree=MozillaTry&rev=063b483f2742 is Linux liking it fine.

Comment 3

[Jason Evans]

Comment on attachment 497309 [details] [diff] [review]
proposal a - remove loop

Looks like a fine change to me.  My guess is that this is leftover cruft from the memory reserve feature.

Comment 4

[Benjamin Smedberg]

Comment on attachment 497309 [details] [diff] [review]
proposal a - remove loop

Unless we can point to a particular reason to take this to solve a known issue, this should wait.

Comment 5

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3788ab7b5b4b