Refactor various things related to compacting GC

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Move calls to protect/unprotect pages into their own functions in gc/Memory.h.
Attachment #8639889 - Flags: review?(terrence)
Assignee

Comment 2

4 years ago
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)
Assignee

Comment 3

4 years ago
Simplify AreansToUpdate implementation to make it more obvious what's going on.  Make methods private where possible.
Attachment #8639894 - Flags: review?(terrence)
Assignee

Comment 4

4 years ago
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)
Assignee

Comment 5

4 years ago
Posted 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)
Assignee

Comment 6

4 years ago
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+
Assignee

Comment 10

4 years ago
(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.