Closed Bug 819215 Opened 7 years ago Closed 7 years ago

merge NS_IMPL_CYCLE_COLLECTION*_CLASS

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch patch 1 merge them (obsolete) — Splinter Review
Attachment #689549 - Flags: feedback?(continuation)
Attachment #689549 - Flags: feedback?(bugs)
Mike, do you have concerns about this adding static constructors?
Trev, does it help to bug 802442?
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.
Attachment #689549 - Flags: feedback-
Comment on attachment 689549 [details] [diff] [review]
patch 1 merge them

Waiting for an approach which doesn't create static ctors.
Attachment #689549 - Flags: feedback?(bugs)
Attachment #689549 - Flags: feedback?(continuation)
(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?
(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.
> 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.
We need to keep the non static member function around for now because of
the xpconnect stuff which doesn't use inner classes.
Attachment #689549 - Attachment is obsolete: true
Attachment #690133 - Flags: feedback?(mh+mozilla)
Attachment #690133 - Flags: feedback?(continuation)
Attachment #690133 - Flags: feedback?(bugs)
Attachment #690132 - Flags: feedback?(continuation)
Attachment #690132 - Flags: feedback?(bugs)
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 :)
Attachment #690133 - Flags: feedback?(mh+mozilla) → feedback+
sorry about all the whitespace errors I made in advance :/
Attachment #690132 - Attachment is obsolete: true
Attachment #690133 - Attachment is obsolete: true
Attachment #690132 - Flags: feedback?(continuation)
Attachment #690132 - Flags: feedback?(bugs)
Attachment #690133 - Flags: feedback?(continuation)
Attachment #690133 - Flags: feedback?(bugs)
Attachment #691528 - Flags: review?(continuation)
Attachment #691528 - Flags: review?(bugs)
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.
Attachment #691528 - Flags: review?(continuation) → review+
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.
Attachment #691528 - Flags: review?(bugs)
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/
Attachment #692011 - Flags: review?(bugs) → review+
This work increases my personal happiness.
https://hg.mozilla.org/mozilla-central/rev/5ef3d98bc229
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 822289
You need to log in before you can comment on or make changes to this bug.