1362956 - Moving array elements can be slow due to GC barriers

Status: RESOLVED DUPLICATE of bug 1348772

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

It looks like we fire a lot of GC barriers when moving array elements around, specifically:

1. During an incremental GC we need to invoke the pre-barrier on all overwritten elements due to the fact that we may be in the middle of incrementally marking the array.

2. The post barrier for generational GC scan the all modified elements looking for nursery pointers.

This can cause jank when modifying large arrays.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I wonder whether we could alleviate this to some extent if push/pop didn't actually move anything in memory but just changed the array start/end positions.  I assume we've considered that anyway to make push/pop faster, right?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #1)
> I wonder whether we could alleviate this to some extent if push/pop didn't
> actually move anything in memory but just changed the array start/end
> positions.  I assume we've considered that anyway to make push/pop faster,
> right?

For shift() we have bug 1348772.

Jon, if this is indeed happening a lot for shift calls, I can prioritize that bug...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Maybe it was shift() that was showing up with the barriers in my profiles, not push/pop.  That would make some sense.... I'll try to reprofile the next time this comes up.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> 2. The post barrier for generational GC scan the all modified elements
> looking for nursery pointers.

Are we always using the elements barrier[0] rather than the whole cell barrier for arrays? If not, I expect we could get some wins by moving more things over.

I guess we must be because all the array methods are self-hosted now right?

[0] http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/js/src/jit/VMFunctions.cpp#681

Comment 5

[Jon Coppeard (:jonco)]

Attachment: array-bench.js

Microbenchmark to compare shift against pop for large arrays.

Length:      Iterations:  GC state:    Contents:    Shift time:  Pop time:   
100000       1000         no GC        objects      0.14057      0.0013379   
100000       1000         no GC        numbers      0.11656      0.0011770   
100000       1000         in GC        objects      2.5354       0.0011880   
100000       1000         in GC        numbers      0.58563      0.0011379   

From this it seems that using shift while in an incremental GC is particularly bad.

The comment in NativeObject::moveDenseElements explains why we currently have to trigger the pre-barrier on every moved element, not just ones that are overwritten:

http://searchfox.org/mozilla-central/source/js/src/vm/NativeObject-inl.h#175

Jan, bug 1348772 could totally solve this (the GC marking would have to subtract numShiftedElements when resuming marking elements).

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
The problem here seems to be the pre-barrier for incremental GC rather than the post-barrier.  I can't find where the JIT triggers a post barrier for this, but certainly we use the slots/elements store buffer for this in the interpreter (see NativeObject::elementsRangeWriteBarrierPost).

Comment 7

[Jan de Mooij [:jandem]]

See the results in bug 1348772 comment 4, it should fix the shift() case.

Comment 8

[Jon Coppeard (:jonco)]

Fixed by the changes in bug 1348772.