GC: indirect Value marking

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 596840 [details] [diff] [review]
v0

This patch reworks the Value marking interfaces to take Value* and HeapValue*.

This appears to basically work and not make anything dramatically more ugly.  The only question I have is what to do about IncrementalValueBarrier.  For now I've updated it to take a pointer as well.
Attachment #596840 - Flags: review?(wmccloskey)
Comment on attachment 596840 [details] [diff] [review]
v0

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

Looks good.

::: js/src/builtin/MapObject.cpp
@@ +185,5 @@
>      MapObject *mapobj = static_cast<MapObject *>(obj);
>      if (ValueMap *map = mapobj->getData()) {
>          for (ValueMap::Range r = map->all(); !r.empty(); r.popFront()) {
> +            const HeapValue &key = r.front().key;
> +            HeapValue orig(key);

Why do you need orig here? Can't you just use key? Same question below.

::: js/src/gc/Barrier-inl.h
@@ +114,5 @@
>      post(comp);
>  }
>  
>  inline void
> +HeapValue::writeBarrierPre(Value *value)

Why change this? We don't ever expect an incremental barrier to move anything.

::: js/src/jscntxt.h
@@ +1132,5 @@
>          JS_ASSERT(link);
>          return reinterpret_cast<JSContext *>(uintptr_t(link) - offsetof(JSContext, link));
>      }
>  
> +    void mark(JSTracer *trc);

There should be an empty line before private:.

::: js/src/jsexn.cpp
@@ +432,5 @@
>              vcount += elem->argc;
>          }
>          vp = GetStackTraceValueBuffer(priv);
>          for (i = 0; i != vcount; ++i, ++vp) {
> +            MarkValue(trc, vp, "stack trace argument");

Please remove the braces while you're here.

::: js/src/jsfriendapi.cpp
@@ +577,5 @@
>          JS_NOT_REACHED("invalid trace kind");
>  }
>  
>  extern JS_FRIEND_API(void)
> +IncrementalValueBarrier(Value *v)

Again, I don't think we need to change this.
Attachment #596840 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 597897 [details] [diff] [review]
final

(In reply to Bill McCloskey (:billm) from comment #1)
> ::: js/src/builtin/MapObject.cpp
> @@ +185,5 @@
> >      MapObject *mapobj = static_cast<MapObject *>(obj);
> >      if (ValueMap *map = mapobj->getData()) {
> >          for (ValueMap::Range r = map->all(); !r.empty(); r.popFront()) {
> > +            const HeapValue &key = r.front().key;
> > +            HeapValue orig(key);
> 
> Why do you need orig here? Can't you just use key? Same question below.

You are quite right.  It took me quite awhile to figure out how to get to the HeapValue out of the HashableValue at all: I must have forgotten to remove the intermediary I was using.

> ::: js/src/gc/Barrier-inl.h
> @@ +114,5 @@
> >      post(comp);
> >  }
> >  
> >  inline void
> > +HeapValue::writeBarrierPre(Value *value)
> 
> Why change this? We don't ever expect an incremental barrier to move
> anything.

I'll definitely undo these.  My thinking was that it might be useful in the nebulous future if we want to have an incremental moving collector.  Ignoring the fact that this is bonkers on its face, it turns out that it also makes for an annoyingly large amount of code motion for the Unbarriered markers.  This will fit better in a different bug anyway, if we do want to do it later.
Attachment #596840 - Attachment is obsolete: true
Attachment #597897 - Flags: review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6f9de44583

Comment 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6f6f9de44583
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.