Closed Bug 687843 Opened 9 years ago Closed 8 years ago

WeakMap: Add MarkOf and factor MarkPolicies.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file, 9 obsolete files)

1.44 KB, patch
nbp
: review+
mccr8
: checkin+
Details | Diff | Splinter Review
+++ 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.
Attachment #561188 - Attachment is patch: true
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.
"WN"?
Sorry, that's my made-up shorthand for "XPC Wrapped Native".  I meant the fixes that went in for Bug 655297.
Blocks: 685841
No longer depends on: 685841
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);
> }
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.
Attachment #561188 - Attachment is obsolete: true
Attachment #565494 - Flags: review?(continuation)
Attachment #565494 - Flags: review?(jorendorff)
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.
Attachment #565494 - Flags: review?(continuation)
rebased.
Attachment #565494 - Attachment is obsolete: true
Attachment #565494 - Flags: review?(jorendorff)
Attachment #567794 - Flags: review?(jorendorff)
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.
Attachment #567794 - Flags: review?(jorendorff) → review?(wmccloskey)
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?
Attachment #567794 - Flags: review?(wmccloskey)
Attachment #567794 - Attachment is obsolete: true
Attachment #568071 - Flags: review?(wmccloskey)
Assignee: general → npierron
Status: NEW → ASSIGNED
Attachment #568073 - Flags: review?(continuation)
Attachment #568073 - Attachment is obsolete: true
Attachment #568073 - Flags: review?(continuation)
Attachment #568086 - Flags: review?(continuation)
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.
Attachment #568071 - Flags: review?(wmccloskey) → review+
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++.
Fix comment.
Attachment #568071 - Attachment is obsolete: true
Attachment #570854 - Flags: review+
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:"
Attachment #568086 - Flags: review?(continuation) → review+
r=me with those fixed.
Version: unspecified → Trunk
Apply fixes from mccr8.
Attachment #568086 - Attachment is obsolete: true
Attachment #570888 - Flags: review+
(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 ?
https://hg.mozilla.org/mozilla-central/rev/b1818463edc1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: 680315, 685841
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.
Attachment #570888 - Attachment is obsolete: true
Attachment #571046 - Flags: review?(continuation)
Attachment #570854 - Flags: checkin+
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&.
(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.
Fix compilation.
Attachment #571046 - Attachment is obsolete: true
Attachment #571046 - Flags: review?(continuation)
Attachment #571056 - Flags: review?(continuation)
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.
Attachment #571056 - Flags: review?(continuation) → review+
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.
Attachment #571056 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.