Closed Bug 1188408 Opened 5 years ago Closed 5 years ago

Refactor various things related to compacting GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

No description provided.
Move calls to protect/unprotect pages into their own functions in gc/Memory.h.
Attachment #8639889 - Flags: review?(terrence)
Rearrange the relocation overlay so that the 'next' field (unused during compacting GC) overlays JSObject::group_.  This means that obj->is<FooObject>() checks will still work on relocaated objects and this allows us to remove some duplicated typed object code which had two separate paths depending on whether an object might be relocated.
Attachment #8639892 - Flags: review?(terrence)
Simplify AreansToUpdate implementation to make it more obvious what's going on.  Make methods private where possible.
Attachment #8639894 - Flags: review?(terrence)
Make incremental zeal mode trigger a shrinking GC and reset the slice budget when we get to the compacting phase so that this exercises incremental compacting GC.
Attachment #8639896 - Flags: review?(terrence)
Attached patch 5-udpate-cell-pointers-at-end (obsolete) — Splinter Review
Change compacting to update all roots before updating cells that may contain stale references to relocated objects.

This allows us to get rid of the hack in SavedFrame where it kept two copies of the parent object pointer so it could tell when it had been moved (one was in a private slot, the other was a normal object value).

I factored out some common code between BaselineFrame and InterpreterFrame that also needed to check for forwarding.
Attachment #8639900 - Flags: review?(terrence)
This bitrotted overnight with the SavedFrame changes.
Attachment #8639900 - Attachment is obsolete: true
Attachment #8639900 - Flags: review?(terrence)
Attachment #8640391 - Flags: review?(terrence)
Comment on attachment 8639889 [details] [diff] [review]
1-refactor-mprotect

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

That's much nicer!

::: js/src/gc/Memory.cpp
@@ +757,5 @@
> +    MOZ_ASSERT(size % pageSize == 0);
> +#if defined(XP_WIN)
> +    DWORD oldProtect;
> +    if (!VirtualProtect(p, size, PAGE_NOACCESS, &oldProtect))
> +        MOZ_CRASH();

We'd better add failure strings to these crashes -- I'm sure one of them will get hit eventually.
Attachment #8639889 - Flags: review?(terrence) → review+
Comment on attachment 8639892 [details] [diff] [review]
2-rearrange-relocation-overlay

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

::: js/src/builtin/TypedObject.h
@@ -423,5 @@
>  
> -    TypeDescr& maybeForwardedElementType() const {
> -        JSObject* elemType = &getReservedSlot(JS_DESCR_SLOT_ARRAY_ELEM_TYPE).toObject();
> -        return MaybeForwarded(elemType)->as<TypeDescr>();
> -    }

\o/

::: js/src/jsgc.h
@@ +1205,5 @@
>          newLocation_ = cell;
>          magic_ = Relocated;
> +    }
> +
> +    RelocationOverlay*& nextRef() {

I think we should probably MOZ_ASSERT(isForwarded()) and MOZ_ASSERT(newLocation_->chunk()->trailer.runtime->gc.incrementalPhase != COMPACT) here and in next().
Attachment #8639892 - Flags: review?(terrence) → review+
Comment on attachment 8639894 [details] [diff] [review]
3-refactor-arenas-iteration

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

Nice!
Attachment #8639894 - Flags: review?(terrence) → review+
Attachment #8639896 - Flags: review?(terrence) → review+
Attachment #8640391 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #8)

> ::: js/src/jsgc.h
> @@ +1205,5 @@
> >          newLocation_ = cell;
> >          magic_ = Relocated;
> > +    }
> > +
> > +    RelocationOverlay*& nextRef() {
> 
> I think we should probably MOZ_ASSERT(isForwarded()) and
> MOZ_ASSERT(newLocation_->chunk()->trailer.runtime->gc.incrementalPhase !=
> COMPACT) here and in next().

Good idea.  It seems we don't have the definition of JSRuntime at this point so we can't do the latter though.
You need to log in before you can comment on or make changes to this bug.