Last Comment Bug 601234 - avoid an extra indirection and a branch on the fast path of GC thing allocation
: avoid an extra indirection and a branch on the fast path of GC thing allocation
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 605029 658016
Blocks: 601075
  Show dependency treegraph
 
Reported: 2010-10-01 14:01 PDT by Igor Bukanov
Modified: 2011-05-23 14:10 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.46 KB, patch)
2010-10-20 12:21 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (11.69 KB, patch)
2011-04-17 15:40 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v3 (11.15 KB, patch)
2011-04-19 15:26 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v4 (8.75 KB, patch)
2011-05-06 13:55 PDT, Igor Bukanov
wmccloskey: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2010-10-01 14:01:14 PDT
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] 2010-10-01 14:19:45 PDT
Yes that's also something I wanted to change but other other fixes were more important. Lets measure the difference.
Comment 2 Igor Bukanov 2010-10-20 12:21:32 PDT
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.
Comment 3 Igor Bukanov 2010-10-22 02:45:10 PDT
Comment on attachment 484777 [details] [diff] [review]
v1

Gregor do not have time for this
Comment 4 Igor Bukanov 2011-04-17 15:40:26 PDT
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.
Comment 5 Igor Bukanov 2011-04-19 15:26:21 PDT
Created attachment 527129 [details] [diff] [review]
v3

rebased patch
Comment 6 Igor Bukanov 2011-05-06 13:55:36 PDT
Created attachment 530730 [details] [diff] [review]
v4

here is a rebased patch
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-05-08 02:37:47 PDT
s/namesspace/namespace/
Comment 8 Bill McCloskey (:billm) 2011-05-12 15:45:41 PDT
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 2011-05-12 16:13:47 PDT
(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 (:billm) 2011-05-12 16:19:27 PDT
Yeah, sounds good. Thanks.
Comment 12 Thomas Ahlblom 2011-05-18 22:44:22 PDT
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 2011-05-19 06:37:54 PDT
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) 2011-05-23 14:10:05 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/0621dbdec4c2

Note You need to log in before you can comment on or make changes to this bug.