347645 - Removal of global arena cache list

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently when allocating a new arena the global list is checked before using malloc. This assumes that allocating from the list is faster than using malloc. But this may not be true for 2 reasons. First, many modern malloc/free implementations use thread-local cache. In such implementation malloc avoids taking any locks in many cases and can bit allocating from the list which allows take the lock. Second, the implementation searches the list for an element with exactly requested size. Since the list is not bounded, this can be arbitrary slow. 

Thus I suggest to remove the global list and always use malloc/free when allocating/releasing arena memory and allow the system to take better use of the memory.

But In case of SpiderMonkey the case is much more stronger for the following reason. The global list is populated only when JS_FreeArenaPool is called, in the rest of cases arena memory is released calling free directly. Now the only place in the browser code that calls JS_FreeArenaPool is js_GetPrinterOutput from js/src/jsopcode.c. 

Since the list is queried for exact allocations, the result of releasing the memory in js_GetPrinterOutput would, in most cases, be only usable when js_NewPrinter is called later. This happens because js_NewPrinter calls JS_InitArenaPool(&jp->pool, name, 256, 1) while in the rest of places JS_InitArenaPool is called with minimal allocation size set to 1K, 8K or the size deduced from the expected buffer size. Thus the global list just imposes useless locking and list searching before calling the malloc.

Here is stats from the browser collected with JS_ARENAMETER defined after the browser startup:

temp allocation statistics:
              number of arenas: 0
         number of allocations: 120748
 number of free arena reclaims: 0
        number of malloc calls: 5012
       number of deallocations: 0
  number of allocation growths: 19
    number of in-place growths: 0
 number of realloc'ing growths: 0
number of released allocations: 4381
       number of fast releases: 3253
         total bytes allocated: 4846989
          mean allocation size: 40.1414
            standard deviation: 186.675
       maximum allocation size: 4096

stack allocation statistics:
              number of arenas: 0
         number of allocations: 420
 number of free arena reclaims: 0
        number of malloc calls: 117
       number of deallocations: 0
  number of allocation growths: 0
    number of in-place growths: 0
 number of realloc'ing growths: 0
number of released allocations: 1012
       number of fast releases: 895
         total bytes allocated: 11584
          mean allocation size: 27.581
            standard deviation: 112.446
       maximum allocation size: 1676

properties allocation statistics:
              number of arenas: 51
         number of allocations: 12898
 number of free arena reclaims: 0
        number of malloc calls: 51
       number of deallocations: 0
  number of allocation growths: 0
    number of in-place growths: 0
 number of realloc'ing growths: 0
number of released allocations: 0
       number of fast releases: 0
         total bytes allocated: 361144
          mean allocation size: 28
            standard deviation: 27.8314
       maximum allocation size: 28

Which tells that there there were 5012 + 117 + 51 useless locking/unlocking calls.

Comment 1

[Igor Bukanov]

Attachment: Implementation v1

Removal of the global list and lock.

Comment 2

[Brendan Eich [:brendan]]

Thanks for filing this -- it's long overdue.  We shoult try to get it into moz1.8 / fx2 / js1.7 too.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 232438 [details] [diff] [review]
Implementation v1

Requesting additional review to buttress case for branch landing.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 232438 [details] [diff] [review]
Implementation v1

This is a safe patch for 1.8.1, because it removes a layer of recycling above malloc.  Unless that pooling hid a buffer overrun bug, removing it will only help performance (time in terms of direct, thread-local malloc calling; and space, due to reduced "external fragmentation" due to hoarding of unusably-sized arenas on the freelist).

We have no evidence of buffer overrun latent bugs, and we should be able to smoke any out with valgrind and other testing.  The odds seem low enough to make to take this patch sooner rather than later.  Igor, what do you think?

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 232438 [details] [diff] [review]
Implementation v1

Neat!

Comment 6

[Igor Bukanov]

Attachment: Implementation v2

In the previous patch I forgot to remove from jsarena.c
include "jslock.h"
which is no longer needed for arena code.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 232775 [details] [diff] [review]
Implementation v2

Should bake for a few days on trunk, get in for beta 2.

/be

Comment 8

[Igor Bukanov]

I committed to the trunk the patch from comment 6.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 232775 [details] [diff] [review]
Implementation v2

a=drivers, so say we all

Comment 10

[Igor Bukanov]

I committed the patch from comment 6 to MOZILLA_1_8_BRANCH