merge NS_IMPL_CYCLE_COLLECTION*_CLASS

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 689549 [details] [diff] [review]
patch 1 merge them
(Assignee)

Updated

6 years ago
Attachment #689549 - Flags: feedback?(continuation)
Attachment #689549 - Flags: feedback?(bugs)
(Assignee)

Comment 2

6 years ago
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 5

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

Updated

6 years ago
Attachment #689549 - Flags: feedback?(continuation)
(Assignee)

Comment 6

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

Comment 8

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

Comment 9

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

Comment 10

6 years ago
Created attachment 690133 [details] [diff] [review]
make the participant object static to the GetParticipant() static functions, and get rid of the global static objects
(Assignee)

Updated

6 years ago
Attachment #690133 - Flags: feedback?(mh+mozilla)
Attachment #690133 - Flags: feedback?(continuation)
Attachment #690133 - Flags: feedback?(bugs)
(Assignee)

Updated

6 years ago
Attachment #690132 - Flags: feedback?(continuation)
Attachment #690132 - Flags: feedback?(bugs)
(Assignee)

Comment 11

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

Comment 12

6 years ago
Created 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

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

Comment 15

6 years ago
Created 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

I think I got all the \s right.
Attachment #692011 - 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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Blocks: 822289
Blocks: 749370
You need to log in before you can comment on or make changes to this bug.