Moving array elements can be slow due to GC barriers

RESOLVED DUPLICATE of bug 1348772

Status

()

enhancement
RESOLVED DUPLICATE of bug 1348772
2 years ago
2 years ago

People

(Reporter: jonco, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.36 KB, application/x-javascript
Details
(Reporter)

Description

2 years ago
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.
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?
(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...
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.
(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
(Reporter)

Comment 5

2 years ago
Posted file 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).
(Reporter)

Comment 6

2 years ago
(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).
Depends on: 1348772
See the results in bug 1348772 comment 4, it should fix the shift() case.
(Reporter)

Comment 8

2 years ago
Fixed by the changes in bug 1348772.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1348772
You need to log in before you can comment on or make changes to this bug.