handling of chunk in arena_run_alloc while loop is odd

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks 1 bug, {coverity})

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

Assignee

Description

9 years ago
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 }
Assignee

Comment 1

9 years ago
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
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #497309 - Flags: review?(jasone)

Comment 3

9 years ago
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.
Attachment #497309 - Flags: review?(jasone) → review+
Assignee

Updated

9 years ago
Attachment #497309 - Flags: approval2.0?

Comment 4

9 years ago
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.
Attachment #497309 - Flags: approval2.0? → approval2.0-

Comment 5

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3788ab7b5b4b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.