819215 - merge NS_IMPL_CYCLE_COLLECTION*_CLASS

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar to bug 806279 but for the NS_IMPL_CYCLE_COLLECTION_*_CLASS macros.  Doing this allows us to get rid ofthe difference between NS_IMPL_CYCLE_COLLECTION_NATIVE_N and NS_IMPL_CYCLE_COLLECTION_N and  should mean that we can use the nsWrapperCache CC macros for non-nsISupports new bindings objects.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch 1 merge them

Comment 2

[Trevor Saunders (:tbsaunde)]

Mike, do you have concerns about this adding static constructors?

Comment 3

[alexander :surkov (:asurkov)]

Trev, does it help to bug 802442?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 689549 [details] [diff] [review]
patch 1 merge them

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

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +120,3 @@
>   *
> + *   CCParticipantVTable<T>::type T::_cycleCollectorGlobal =
> + *   CCParticipantVTableHolder<T>::array;

s/array/functions/ but yeah... that creates static constructors. Even worse, it makes the memory footprint bigger because it creates two tables of functions, and copies one into the other at runtime (with the static constructors). What you can try is to completely remove _cycleCollectorGlobal and use CCParticipantVTableHolder<T>::functions directly, if that works.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 689549 [details] [diff] [review]
patch 1 merge them

Waiting for an approach which doesn't create static ctors.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 689549 [details] [diff] [review]
> patch 1 merge them
> 
> Review of attachment 689549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/nsCycleCollectionParticipant.h
> @@ +120,3 @@
> >   *
> > + *   CCParticipantVTable<T>::type T::_cycleCollectorGlobal =
> > + *   CCParticipantVTableHolder<T>::array;
> 
> s/array/functions/ but yeah... that creates static constructors. Even worse,
> it makes the memory footprint bigger because it creates two tables of
> functions, and copies one into the other at runtime (with the static

ugh! silly compiler :(  What exactly causes static constructors here / in what cases will gcc precompute things and just put it all in rodata?

> constructors). What you can try is to completely remove
> _cycleCollectorGlobal and use CCParticipantVTableHolder<T>::functions

I'm not sure I understand what you mean by removing _CycleCollectorGlobal.

> directly, if that works.

well, one option would be to make the global itself an empty object  that only has a member function GetParticipant which actually stores the function table as a function static object, would that be better?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Comment on attachment 689549 [details] [diff] [review]
> > patch 1 merge them
> > 
> > Review of attachment 689549 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: xpcom/glue/nsCycleCollectionParticipant.h
> > @@ +120,3 @@
> > >   *
> > > + *   CCParticipantVTable<T>::type T::_cycleCollectorGlobal =
> > > + *   CCParticipantVTableHolder<T>::array;
> > 
> > s/array/functions/ but yeah... that creates static constructors. Even worse,
> > it makes the memory footprint bigger because it creates two tables of
> > functions, and copies one into the other at runtime (with the static
> 
> ugh! silly compiler :(  What exactly causes static constructors here / in
> what cases will gcc precompute things and just put it all in rodata?

Essentially, what you're doing is this:

struct A {
  void *a[20];
};

const A a = { };
const A b = a;


Even if the compiler avoided the static initializer, you'd still have the data taking twice as much space as it needs.
 
> > constructors). What you can try is to completely remove
> > _cycleCollectorGlobal and use CCParticipantVTableHolder<T>::functions
> 
> I'm not sure I understand what you mean by removing _CycleCollectorGlobal.

In the above example, that would be getting rid of the need of b, somehow.

> 
> > directly, if that works.
> 
> well, one option would be to make the global itself an empty object  that
> only has a member function GetParticipant which actually stores the function
> table as a function static object, would that be better?

Note there is already a GetParticipant member function. The problem with empty objects is that they still take space (they need an address).

I wonder if just using CCParticipantVTableHolder<T>::functions where we currently use T::_CycleCollectorGlobal wouldn't work.

Comment 8

[Trevor Saunders (:tbsaunde)]

> struct A {
>   void *a[20];
> };
> 
> const A a = { };
> const A b = a;
> 
> 
> Even if the compiler avoided the static initializer, you'd still have the
> data taking twice as much space as it needs.

well, I'd hope in a ideal world that the tool chain would just merge the two objects, but I guess that's a bit optamistic.

> > 
> > > directly, if that works.
> > 
> > well, one option would be to make the global itself an empty object  that
> > only has a member function GetParticipant which actually stores the function
> > table as a function static object, would that be better?
> 
> Note there is already a GetParticipant member function. The problem with
> empty objects is that they still take space (they need an address).

does that address need to be unique / dereferenceable?

> I wonder if just using CCParticipantVTableHolder<T>::functions where we
> currently use T::_CycleCollectorGlobal wouldn't work.

its worth a try, but I'm not sure if it'll work for the xpcom non native case.

Comment 9

[Trevor Saunders (:tbsaunde)]

Attachment: part 1 give cycleCollection inner classes a static GetParticipant() method.

We need to keep the non static member function around for now because of
the xpconnect stuff which doesn't use inner classes.

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

Comment 11

[Trevor Saunders (:tbsaunde)]

Note with these patches we don't actually need NS_IMPL_CYCLE_COLLECTION_CLASS at all, so once this work is done I'll generate patches to remove it and the similar macros from the tree unless Andrew wants to do more patches like that :)

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 819215 - make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

sorry about all the whitespace errors I made in advance :/

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 691528 [details] [diff] [review]
bug 819215 - make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

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

Thanks for fixing this!

nit: Please remove the definition of NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE from nsCycleCollectionParticipant.h, as it looks like you've removed all uses of it.

These GetParticipant declarations look like they could be macroized to condense things a bit. But that can just happen in a later bug.

I don't fully understand all the static const weirdness going on here, but glandium has looked at it so it should be okay.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 691528 [details] [diff] [review]
bug 819215 - make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

There is a new patch coming.

Comment 15

[Trevor Saunders (:tbsaunde)]

Attachment: bug 819215 - make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

I think I got all the \s right.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 692011 [details] [diff] [review]
bug 819215 - make the participant object static to the GetParticipant() static functions, and get rid of the global static objects

s/NS_RETURN_PARTICIPANT_AS/NS_PARTICIPANT_AS/

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef3d98bc229

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

This work increases my personal happiness.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5ef3d98bc229