The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
Yes that's also something I wanted to change but other other fixes were more important. Lets measure the difference.
(Assignee)

Updated

7 years ago
Depends on: 605029
(Assignee)

Comment 2

7 years ago
Created attachment 484777 [details] [diff] [review]
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.
Attachment #484777 - Flags: review?(anygregor)
(Assignee)

Comment 3

7 years ago
Comment on attachment 484777 [details] [diff] [review]
v1

Gregor do not have time for this
Attachment #484777 - Flags: review?(anygregor) → review?(gal)
(Assignee)

Comment 4

6 years ago
Created attachment 526636 [details] [diff] [review]
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.
Attachment #484777 - Attachment is obsolete: true
Attachment #526636 - Flags: review?(gal)
Attachment #484777 - Flags: review?(gal)
(Assignee)

Comment 5

6 years ago
Created attachment 527129 [details] [diff] [review]
v3

rebased patch
Attachment #526636 - Attachment is obsolete: true
Attachment #527129 - Flags: review?(gal)
Attachment #526636 - Flags: review?(gal)
(Assignee)

Comment 6

6 years ago
Created attachment 530730 [details] [diff] [review]
v4

here is a rebased patch
Attachment #527129 - Attachment is obsolete: true
Attachment #530730 - Flags: review?(wmccloskey)
Attachment #527129 - Flags: review?(gal)
s/namesspace/namespace/
(Assignee)

Updated

6 years ago
Blocks: 601075
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.
Attachment #530730 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

6 years ago
(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?
Yeah, sounds good. Thanks.
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/tracemonkey/rev/d406a64628e3
Whiteboard: fixed-in-tracemonkey

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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.
Depends on: 658016
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0621dbdec4c2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.