170895 - nsComputedDOMStyle and nsDOMCSSDeclaration should share CSS2Properties code

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

http://bugzilla.mozilla.org/show_bug.cgi?id=117500#c5
A fun followup patch would be to use COM aggregation to reduce code size by
sharing all the CSS2Properties stubs between the two implementations

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Talking with roc and dbradley about using libxptcall here.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

So if we go the libxptcall route, it looks like this is blocked by bug 54471. 
Adding dependency.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Okay, so I went ahead and got the actual figures (on Linux at least). I measured
libgkcontent's size four times:

1) Locally backed out my landing of 117500
2) Re-added the .idl changes and added the necessary macros to
nsDOMCSSDeclaration.cpp
3) Tested against the current trunk (the full fix for 117500)
4) After talking with jst, he suggested a tearoff.  I wrote one and tested
against that.

The absolute change in my table below is the change in size from before the fix
to 117500.  The effective change is the change in size from step 2 above.  It
measures change against the current IDL state, so that is a more realistic
figure to go by (although the difference between that and the absolute change
isn't great).

                  |   Size   |  Absolute Change | Effective Change
------------------|----------|------------------|-----------------
Before 117500     |  6117135 |                0 |            -8988
After IDL changes |  6126123 |             8988 |                0
Current Trunk     |  6221409 |           104274 |            95286
With Tearoff      |  6080154 |           -36981 |           -45969


So, we inflated over 100k with the checkin.  However, I can get that all back
and then some (141k win from trunk) by using a tearoff as jst suggested.  Since
this works well without going through the scarier route of xptcall, which would
require considerably more work and testing on a bunch of platforms, I'm in favor
of avoiding that and just going the tearoff route.

Patch to follow.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v1.0

Comment 5

[Johnny Stenback (:jst)]

This sounds awesome to me. I didn't look at the diff yet, but using xptcall for
this sounds scary and unnecessary. Great work caillon!

Comment 6

[Alec Flett]

while I love the idea, the one problem I see here is that you're creating a new
tearoff for every QI() - not only does that sound unnecessary, but it violates
the rules of COM - if I 

(A.QI(nsIB) == B) => B.QI(nsIB) == A

but since you're getting a new interface, your result is more like
(A.QI(nsIB) == B) => B.QI(nsIB) == C, C != A

To do proper aggregation, check out the Don Box book, or look for some examples
in our code. I think there are at least 2 or 3.
http://lxr.mozilla.org/seamonkey/search?string=impl_aggreg

Comment 7

[Brendan Eich [:brendan]]

I thought COM required only that interfaces implemented by the same object QI to
the same nsISupports -- not that QI to any interface other than nsISupports be
symmetric or transitive among the set of all non-nsISupports interfaces
implemented by an object (possibly using aggregation).

/be

Comment 8

[Johnny Stenback (:jst)]

I we are actually breaking the COM rules here, but we've been doing that forever
(we do it for all other tearoff's in the content code at least) and we've talked
about it before and so far we haven't cared enough to unbreak things. It saves
us a reference to the tearoff (which can be pushed out into a hash to save size
on the object if the tearoff is rarely used), and it doesn't matter for any of
the code that uses this in mozilla.

Comment 9

[Alec Flett]

hmm. It might save you the 4 bytes in the class, but you still potentially have
multiple instances of the tearoff, so my guess is there's no real space savings
here...

Why not just do something like

nsCSS2PropertiesTearoff*
GetTearoff() {
  if (!mCSS2Tearoff) mCSS2Tearoff = new nsCSS2PropertiesTearoff(this);
  return mCSS2Tearoff;
}

if there are multiple tearoffs of this class, use an nsVoidArray with well-known
indexes or something. (i.e. 0 = properties, 1 = ??, 2 = ??, etc)

I guess its the new'ing of an object every time that bothers me the most, and
COM rules seemed like a good justification not to do it.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

The only real caller of this is content JS through XPConnect which (AIUI) caches
the instance until it is GC'd.  There is no good reason for C++ to call this, as
it is only a wrapper around CSSStyleDeclaration.  Accessing an attribute of the
interface just calls the appropriate method on the other interface.  So we
really should only have one instance at a time...

Comment 11

[Johnny Stenback (:jst)]

For C++ users you can cause multiple tearoff's per instance if you're not
re-using the tearoff and QI'ing over and over again, but in all cases in our
codebase the cache is not needed (as caillon pointed out, XPConnect will cache
the tearoff, and it will QI only once during the lifetime of a wrapper).

IMO it's not worth the 4 bytes per instalce, if anything, we should have a
global hash from instance to tearoff.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why does everyone think xptcall is scary? :-)

Comment 13

[Johnny Stenback (:jst)]

Scary is a relative term, compared to writing this in plain ol' C++ that we all
know and love :-), xptcall is rather scary...

Comment 14

[David Bradley]

One side effect of this is XPConnect would create multiple wrappers since the QI
back to nsISupports would yield different pointers.

xptcall requires a lot more work and risk than what's currently on the table.

Comment 15

[Johnny Stenback (:jst)]

No, QI from the tearoff back to nsISupports gets you the instance where the
tearoff was gotten from. That's an important COM rule we can't break. If this
patch breaks that rul, it needs some fixin' (which is trivial).

Comment 16

[David Bradley]

I think I got fooled by the K&R style bracing and the diff in
CSS2PropertiesTearoff::QueryInterface and missed the line below, so it looks ok.

+  return mDecl->QueryInterface(aIID, aInstancePtr);

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yes, my patch will get you the nsISupports of the creating instance.  QI'ing
from the tearoff to any other iface will work as expected.  QI'ing TO the
tearoff multiple times will get you different tearoffs but since we cache that
in XPConnect, and have no users in C++ (and if anyone does in the future, they
should be beaten and then converted to use CSSStyleDeclaration) that is not
really a major concern.

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

So what do you guys say?  This is a nice code size reduction that I'd like to
get in.  I don't think it's worth keeping a global hashtable since we should not
have callers of this outside of web content JS (thru XPConnect).  There is
plenty reason to not do that before this patch, and I don't mind adding an extra
reason for people to not use this interface.

David Bradley suggested making note of this in the .idl file in order to keep
people from using it, and I agree that it is a good thing to do.  I'll post
proposed documentation in a second.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: .idl documentation

Goes along with attachment 100871 [details] [diff] [review].

Seeking r/sr= for the two attachments.

Comment 20

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In this case, why do we care about the size of the base objects?  Aren't they
both only created when users are using the CSSOM, in which case they're almost
definitely going to be using this interface?  In other words, why not just do
proper COM aggregation?

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 100871 [details] [diff] [review]
Proposed fix v1.0

>+NS_IMETHODIMP
>+CSS2PropertiesTearoff::QueryInterface(REFNSIID aIID, void** aInstancePtr)
>+{
>+  if (aIID.Equals(NS_GET_IID(nsIDOMCSS2Properties)) ||
>+      aIID.Equals(NS_GET_IID(nsIDOMNSCSS2Properties))) {
>+    nsISupports *inst = this;
>+
>+    NS_ADDREF(inst);
>+
>+    *aInstancePtr = inst;
>+
>+    return NS_OK;
>   }
> 
>+  return mDecl->QueryInterface(aIID, aInstancePtr);

There's no need to hand-write this -- you can use
NS_INTERFACE_MAP_END_AGGREGATED.  I also don't like the assignment to
|aInstancePtr| without an intermediate cast to nsIDOMNSCSS2Properties, even
though it doesn't matter here (at least for most compilers, and probably all). 
(Same goes for the other two QI implementations.)  And even that assumes that
nsIDOMNSCSS2Properties inherits from nsIDOMCSS2Properties.

Comment 22

[Alec Flett]

well, we all seem pretty ok with this so it sounds good to me!

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

If the consensus seems to want proper COM aggregation, then that's fine I
suppose.  It just seems like it's unnecessary.  Yes, webpages will be very
likely to use this interface when we're in the CSSOM.  Yes, the size is very
little.  But we should never have callers outside of web content.  XPConnect
caches the interfaces so we will only have one instance of this interface per
object at any given time.  We are going to protect possible callers of this in
the future, but those callers are already broken just by calling into this
slower, redundant, interface.

I'll come up with a patch, though...

Comment 24

[Johnny Stenback (:jst)]

I say "why bother", but I'm not going to stop anyone who wants to do the right
thing here. If it really matters, we already have problems in several places,
and I don't know of any of those problems yet so I don't see why we'd care here
either. At some point, if it ever does matter, we can go through the codebase
looking at all the tearoffs and fix them all in one pass... But again, I don't
care, size doesn't really matter here, but it does in other places where we use
tearoffs...

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Using proper COM aggregation

So it doesn't matter which one we choose to use.  This is more correct, with an
extra 1k of footprint and 4 bytes per CSSStyleDeclaration.  Either way, we'll
still have a nice win.

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 101221 [details] [diff] [review]
Using proper COM aggregation

r=dbaron

Comment 27

[Alec Flett]

Comment on attachment 101221 [details] [diff] [review]
Using proper COM aggregation

sr=alecf
thanks for doing this the COM friendly way..

Comment 28

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix checked in at 10/03/2002 12:41.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

I said fixed.