601234 - avoid an extra indirection and a branch on the fast path of GC thing allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

With changes from the bug 558861 to allocate a new GC thing the code uses the essentially the following indirection:

T* thing = *cx->compartment->particular_free_list

Note here an extra *dereference. It is necessary since particular_free_list is a pointer into the free list for the given arena, not just a free list head. This extra indirection and the extra branch test it implies can be avoided if the code would use the same approach that pre bug 558861 code was using. There the thing where allocated using cx->thread->particular_free_list without the dereference. To make sure that during finalization the GC will see all free lists JSThread::purge copied the free list back to its arena.

AFAICS this can be reproduces again via moving the corresponding machinery from thread to the compartment.

Comment 1

[Gregor Wagner [:gwagner]]

Yes that's also something I wanted to change but other other fixes were more important. Lets measure the difference.

Comment 2

[Igor Bukanov]

Attachment: v1

The patch implements the above idea. For SS it gives about 2ms win with no regressions with v8.

The patch applies on top of the patch from the bug 605029 comment 2.

Comment 3

[Igor Bukanov]

Comment on attachment 484777 [details] [diff] [review]
v1

Gregor do not have time for this

Comment 4

[Igor Bukanov]

Attachment: v2

Here is unrotten patch. sunspider and v8 shows 0.3% - 0.4% speedup. I also verified that under valgrind an individual benchmarks where sunspider-compare-results shows tiny regressions in fact shows consistent speedup.

Comment 5

[Igor Bukanov]

Attachment: v3

rebased patch

Comment 6

[Igor Bukanov]

Attachment: v4

here is a rebased patch

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

s/namesspace/namespace/

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 530730 [details] [diff] [review]
v4

Review of attachment 530730 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Could you add a comment at the top of the FreeLists class that describes our policy? It took me a little while to understand what was going on. Maybe something like this:

For a given arena, finalizables[thingKind] points to the next object to be allocated. It gets initialized, in RefillTypedFreeList, to the first free cell in an arena. For each allocation, it is advanced to the next free cell in the same arena. While finalizables[thingKind] points to a cell in an arena, that arena's freeList pointer is NULL. Before doing a GC, we copy finalizables[thingKind] back to the arena header's freeList pointer and set finalizables[thingKind] to NULL. Thus, we only have to maintain one free list pointer at any time.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> 
> For a given arena, finalizables[thingKind] points to the next object to be
> allocated. It gets initialized, in RefillTypedFreeList, to the first free
> cell in an arena. For each allocation, it is advanced to the next free cell
> in the same arena. While finalizables[thingKind] points to a cell in an
> arena, that arena's freeList pointer is NULL. Before doing a GC, we copy
> finalizables[thingKind] back to the arena header's freeList pointer and set
> finalizables[thingKind] to NULL. Thus, we only have to maintain one free
> list pointer at any time.

I would extend that with:

Thus, we only have to maintain one free list pointer at any time and avoid accessing and updating the arena header on each allocation. 

Is it OK?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, sounds good. Thanks.

Comment 11

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/d406a64628e3

Comment 12

[Thomas Ahlblom]

There are indications that http://hg.mozilla.org/tracemonkey/rev/d406a64628e3 makes Firefox always crash at exit when running Yandex Bar in text mode.

See bug 657199.

Comment 13

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/0621dbdec4c2 - followup to restore tolerance to calling the allocator during the GC for compatibility with some extensions, see bug 657199 for details.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0621dbdec4c2