WeakMap: Add MarkOf and factor MarkPolicies.

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

1.44 KB, patch
nbp
: review+
mccr8
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 561188 [details] [diff] [review]
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.
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.

Comment 2

6 years ago
"WN"?
Sorry, that's my made-up shorthand for "XPC Wrapped Native".  I meant the fixes that went in for Bug 655297.
(Assignee)

Updated

6 years ago
Blocks: 685841
No longer depends on: 685841
(Assignee)

Comment 4

6 years ago
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);
> }
(Assignee)

Comment 5

6 years ago
Created attachment 565494 [details] [diff] [review]
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.
Attachment #561188 - Attachment is obsolete: true
Attachment #565494 - Flags: review?(continuation)
(Assignee)

Updated

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

Comment 7

6 years ago
Created attachment 567794 [details] [diff] [review]
Add setMark & getMark and MarkPolicyBase classes.

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)
(Assignee)

Comment 10

6 years ago
Created attachment 568071 [details] [diff] [review]
Add gc::Mark and gc::IsMarked functions.
Attachment #567794 - Attachment is obsolete: true
Attachment #568071 - Flags: review?(wmccloskey)
(Assignee)

Comment 11

6 years ago
Created attachment 568073 [details] [diff] [review]
Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.
Assignee: general → npierron
Status: NEW → ASSIGNED
Attachment #568073 - Flags: review?(continuation)
(Assignee)

Comment 12

6 years ago
Created attachment 568086 [details] [diff] [review]
Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.
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++.
(Assignee)

Comment 15

6 years ago
Created attachment 570854 [details] [diff] [review]
Add gc::Mark and gc::IsMarked functions.

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
http://hg.mozilla.org/integration/mozilla-inbound/rev/b1818463edc1
(Assignee)

Comment 19

6 years ago
Created attachment 570888 [details] [diff] [review]
Factor DefaultMarkPolicy and use gc::Mark / gc::IsMarked functions.

Apply fixes from mccr8.
Attachment #568086 - Attachment is obsolete: true
Attachment #570888 - Flags: review+
(Assignee)

Comment 20

6 years ago
(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
Last Resolved: 6 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
(Assignee)

Comment 23

6 years ago
Created attachment 571046 [details] [diff] [review]
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.
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&.
(Assignee)

Comment 25

6 years ago
(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.
(Assignee)

Comment 26

6 years ago
Created attachment 571056 [details] [diff] [review]
Use gc::Mark / gc::IsMarked functions in WeakMap mark policies.

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+
(Assignee)

Comment 28

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

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.