687843 - WeakMap: Add MarkOf and factor MarkPolicies.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: Add MarkOf and MarkPolicyBase classes.

+++ This bug was initially created as a clone of Bug #685841 +++

The MarkOf<Type> class is a way to re-use MarkPolicy classes.  This avoid duplication of MarkPolicy using different types of Key and Value types.  Thus, WeakMap can be used with any object by specializing the MarkOf<MyObject> class.

The MarkPolicyBase is used to factor keyMarked, valueMarked and markEntry (without Bug 684595 modification yet) functions among all MarkPolicies.  This does not seems to be useful in the current patch because it adds more lines, but it is for the patches added as attachment of Bug 685841.

Comment 1

[Andrew McCreight [:mccr8]]

It is kind of a shame that we need a separate MarkPolicy for <JSObject*, Value>, due to the weird WN key problem.  Generally, it is also a little sketchy that in the <JSObject *, JSObject *> case WN's shouldn't be used as keys, because they will disappear, but there's no checking of that.  I'll try to think if we can deal with that in this refactoring or not.

Comment 2

[Jim Blandy :jimb]

"WN"?

Comment 3

[Andrew McCreight [:mccr8]]

Sorry, that's my made-up shorthand for "XPC Wrapped Native".  I meant the fixes that went in for Bug 655297.

Comment 4

[Nicolas B. Pierron [:nbp]]

While discussing naming with [:mccr8] for the MarkOf class, we suggested two set of replacements for the "marked" and "mark" static functions.

either
  Mark<Type>::of(...)
  Mark<Type>::ing(...)

or
  Mark<Type>::get(...)
  Mark<Type>::set(...)

Mark<Type> will have no instances, thus the Mark<Type> prefix will always be used.

On the other hand, we can get rid of the template parameter and of the enclosing class and rely on the overloading of the function to specialize the "markof" and "mark" functions.

> class A {};
> class B {};
> 
> int markof(const A&) {
>   return 1;
> }
> 
> int markof(const B&) {
>   return 2;
> }
> 
> int main ()
> {
>   A a;
>   B b;
>   return markof(a) + markof(b);
> }

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Add setMark & getMark and MarkPolicyBase classes.

I've replaced the MarkOf class and replaced it by using function setMark and getMark which are based on the overloading instead of template specialization.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 565494 [details] [diff] [review]
Add setMark & getMark and MarkPolicyBase classes.

Sorry I forgot about this.  It turns out billm also is planning on doing something like this for incremental GC, so you should coordinate with him.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Add setMark & getMark and MarkPolicyBase classes.

rebased.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 567794 [details] [diff] [review]
Add setMark & getMark and MarkPolicyBase classes.

Jason said it's okay if I review this. It fits in with some stuff I was doing. I'll try to get to it today or tomorrow.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 567794 [details] [diff] [review]
Add setMark & getMark and MarkPolicyBase classes.

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

Thanks, this looks like a nice improvement. If you repost a patch with these issues fixed, I can review it.

::: js/src/jsweakmap.h
@@ +274,5 @@
> +inline bool
> +getMark(JSContext *cx, gc::Cell *cell)
> +{
> +    return !IsAboutToBeFinalized(cx, cell);
> +}

I think getMark and setMark make more sense inside jsgcmark.h. Also, I think they should be named IsMarked and Mark, since we use names like these elsewhere. Also, for future reference, we don't normally use two carriage returns in a file. If you want separation, the usual thing is to add a comment like /*********************/ in between.

@@ +294,5 @@
> +};
> +
> +
> +template <class Key, class Value>
> +class DefaultMarkPolicy : public MarkPolicyBase<Key, Value> {

Is this used anywhere? If not, can we take it out?

@@ +332,5 @@
>      // behavior for this key.
>      bool overrideKeyMarking(JSObject *k) {
>          // We only need to worry about extra marking of keys when
>          // we're doing a GC marking pass.
> +        if (!IS_GC_MARKING_TRACER(this->tracer))

Why the change to this->tracer? Normally we only do this to avoid ambiguity.

@@ +358,1 @@
>      bool markEntryIfLive(gc::Cell *k, JSObject *v) {

It seems like this is basically the same as the inherited implementation. Could we just use that?

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Add gc::Mark and gc::IsMarked functions.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 568071 [details] [diff] [review]
Add gc::Mark and gc::IsMarked functions.

Thanks. Please use one space after a period in the comment.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 568071 [details] [diff] [review]
Add gc::Mark and gc::IsMarked functions.

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

::: js/src/jsgcmark.h
@@ +188,5 @@
>  void
>  MarkChildren(JSTracer *trc, JSXML *xml);
>  
> +/*
> + * Use C function overloading to decide which function should be called based on

Should that be "C++ function overloading"?  Or it could probably just be "function overloading" because it is implicit that we're in C++.

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: Add gc::Mark and gc::IsMarked functions.

Fix comment.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 568086 [details] [diff] [review]
Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.

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

Aside from a few small nits, this looks good to me.

::: js/src/jsweakmap.h
@@ +242,5 @@
>  
> +template <class Key, class Value>
> +class MarkPolicyBase {
> +  protected:
> +    JSTracer *tracer_;

As per IRC discussion, outside of Ionmonkey the trailing underscore style isn't used as far as I can tell, so I think this should be just "tracer", assuming that doesn't conflict.

@@ +254,5 @@
> +    }
> +};
> +
> +template <class Key, class Value>
> +class DefaultMarkPolicy : public MarkPolicyBase<Key, Value> {

Maybe put a "private:" here to be consistent with the rest of the file.  Otherwise the 4 space indentation for base looks weird.

@@ +261,5 @@
> +  public:
> +    DefaultMarkPolicy(JSTracer *t) : base(t) { }
> +    bool markEntryIfLive(Key k, Value v) {
> +        if (keyMarked(k) && !valueMarked(v)) {
> +            // Due to the template on the base class, g++ cannot check for the

I think we usually refer to g++ as "GCC".  Is this a GCC-specific problem?  Will this also work for Visual C++?

@@ +278,5 @@
>  // integration is currently busted anyways.  When WeakMap+CC integration is
>  // fixed in Bug 668855, XPC wrapped natives should only be marked during
>  // non-BLACK marking (ie grey marking).
>  template <>
> +class DefaultMarkPolicy<JSObject *, Value> : public MarkPolicyBase<JSObject *, const Value&> {

same thing here with the "private:"

Comment 17

[Andrew McCreight [:mccr8]]

r=me with those fixed.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1818463edc1

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.

Apply fixes from mccr8.

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Andrew McCreight [:mccr8] from comment #16)
> @@ +261,5 @@
> > +  public:
> > +    DefaultMarkPolicy(JSTracer *t) : base(t) { }
> > +    bool markEntryIfLive(Key k, Value v) {
> > +        if (keyMarked(k) && !valueMarked(v)) {
> > +            // Due to the template on the base class, g++ cannot check for the
> 
> I think we usually refer to g++ as "GCC".  Is this a GCC-specific problem? 
> Will this also work for Visual C++?

I did not test with other compilers, but last time I checked this was only referring to GCC behavior.  On the other hand, I don't mind replacing GCC by "the compiler", this would not change the comment which explain why the "this->" or "base::" is needed here.


Now, Bug 685841 no longer depends on this refactoring.  In its current state it brings usage of gc::Mark function, which is orthogonal to the refactoring with a base class for the policy.

I think the base class is no longer necessary because it was used by Bug 685841 to provide the cache policy.  Maybe we should limit this patch to the usage of gc::Mark or just ignore it for the moment.  What do you think ?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/b1818463edc1

Comment 22

[Andrew McCreight [:mccr8]]

(In reply to Nicolas B. Pierron [:pierron] from comment #20)
> I think the base class is no longer necessary because it was used by Bug
> 685841 to provide the cache policy.  Maybe we should limit this patch to the
> usage of gc::Mark or just ignore it for the moment.  What do you think ?
Yeah, just changing to gc::Mark may be the way to go here.

(reopened because there's another patch we may land)

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

I discard previous reviews due to the major change of this patch.  Now it only use gc::Mark functions and not do refactor Mark policies.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 571046 [details] [diff] [review]
Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

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

::: js/src/jsweakmap.h
@@ +276,5 @@
>          if (keyMarked(k))
>              return markUnmarkedValue(v);
>          if (!overrideKeyMarking(k))
>              return false;
> +        gc::Mark(tracer, *k, "WeakMap entry wrapper key");

Shouldn't this be k instead of *k?  Your other patch defined a Mark for JSObject*, not JSObject&.

Comment 25

[Nicolas B. Pierron [:nbp]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> Comment on attachment 571046 [details] [diff] [review] [diff] [details] [review]
> Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.
> 
> Review of attachment 571046 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsweakmap.h
> @@ +276,5 @@
> >          if (keyMarked(k))
> >              return markUnmarkedValue(v);
> >          if (!overrideKeyMarking(k))
> >              return false;
> > +        gc::Mark(tracer, *k, "WeakMap entry wrapper key");
> 
> Shouldn't this be k instead of *k?  Your other patch defined a Mark for
> JSObject*, not JSObject&.

Indeed.  Apparently I forgot to compile it before submitting.  I'll fix my workflow to prevent this in the future.

Comment 26

[Nicolas B. Pierron [:nbp]]

Attachment: Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

Fix compilation.

Comment 27

[Andrew McCreight [:mccr8]]

Comment on attachment 571056 [details] [diff] [review]
Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

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

Looks good.  This patch is a nice little cleanup.

Comment 28

[Nicolas B. Pierron [:nbp]]

Comment on attachment 571056 [details] [diff] [review]
Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

The patch no longer apply to the code-base since the modification which introduces write barrier.