Closed Bug 114713 Opened 18 years ago Closed 17 years ago

deCOMify nsIStyleContext

Categories

(Core :: CSS Parsing and Computation, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: dbaron, Assigned: bryner)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [whitebox])

Attachments

(2 files, 7 obsolete files)

nsIStyleContext doesn't need to be an XPCOM interface -- it should really be
accessed only within the layout library.  vtune information showed that one of
the places with the highest overhead for virtual function calls was
nsStyleContext::GetStyleData   Also see bug 107100.
This depends on bug 107100 and may also depend on bug 104346, although probably not.
Blocks: deCOM
Depends on: 107100
Status: NEW → ASSIGNED
Depends on: 106161
Depends on: 107101
Keywords: perf
Priority: -- → P1
Target Milestone: --- → Future
cc'ing myself
Attached patch some dependency reduction (obsolete) — Splinter Review
Attached patch more dependency reduction (obsolete) — Splinter Review
This includes all of dbaron's changes plus several more.  A couple of points:

- I wonder if we should consider making a (non-inline) method on nsIFrame to
check style properties such as visibility.  That would avoid the long-winded
code pattern necessary to check the computed style.

- I'm not sure what the best way is to remove the dependency in
embedding/browser/webBrowser/nsContextMenuInfo.cpp, where it actually traverses
the style context tree.  Would this be equivalent to checking the computed
style on each ancestor element?
Attachment #99431 - Attachment is obsolete: true
Attachment #109882 - Flags: review?(dbaron)
Comment on attachment 109882 [details] [diff] [review]
more dependency reduction

Hmmm.  So one of the reasons I never checked in my changes was because I didn't
really like them all that much.

I'm not sure what the rationale for the changes to inspector /
nsInspectorCSSUtils is.  The idea of the latter was to be as small as possible
while still preventing linkage problems -- are there still such problems? 
(But, actually, won't decomifying introduce a whole new series of problems
here?)

Do you think this is the best way to go or do you think we're better off with a
virtual method on nsIFrame (in addition to nsIFrame::GetStyleData)?
Oh, after reading your comment again, I'd agree with you about adding a method
to nsIFrame.

I think the right solution to nsContextMenu.cpp is to scrap the code and make it
call the code that was *copied and pasted* to create that code (code from
nsCSSRendering.cpp), despite my objections at the time.
We could certainly add nsIFrame::GetRuleNode and avoid the extra code in
InspectorCSSUtils.

Could I just use GetStyleData instead of doing the GetComputedStyle thing?  I
wasn't quite sure why you didn't do that in your original patch.
Actually, nevermind, that wouldn't work because the method is inlined.  We could
do something like:

NS_IMETHOD GetStyleDataExternal(...) = 0;
virtual nsRuleNode* GetRuleNode() = 0;

Or we could #ifdef the GetStyleData declaration depending on whether you're in
the layout code (for example, make it an inline call to GetStyleDataExternal
from outside layout).

Regarding the background image code, I couldn't find any code like that in
nsCSSRendering.  But, if we made the additions to nsIFrame I suggested above, it
could walk up the frame tree and check the background style on each one without
adding all the extra code needed for the computed style stuff.
... except that the inspector code has that hack for table frames where it needs
to walk up the style context tree one node more.  So I don't know that this can
be solved entirely by using a GetRuleNode method on nsIFrame, unless we can
assume something about the rule tree (e.g. that both style contexts would point
to the same rule node).
Comment on attachment 109882 [details] [diff] [review]
more dependency reduction

Just thought I'd note that this introduces a major regression in editor -
nsEditor::IsPreformatted fails because text nodes don't QI to nsIDOMElement.
nsIFrame::GetStyleDataExternal (virtual rather than inline) seems like a
reasonable solution.

When you ask me why I didn't do something in the first patch, remember that I
said I didn't like that patch.
Attached patch another try (obsolete) — Splinter Review
I got rid of the DOM computed style stuff in favor of an externally callable
version of GetStyleData and a couple of new methods on nsIPresContext.	This
eliminates _all_ use of nsIStyleContext from outside content and layout.
Attachment #109882 - Attachment is obsolete: true
Attachment #110117 - Flags: review?(dbaron)
Comment on attachment 110117 [details] [diff] [review]
another try

somehow i got two review requests here...
Attachment #110117 - Flags: review?(dbaron)
Attachment #110117 - Flags: review?(dbaron)
Attachment #109882 - Flags: review?(dbaron)
Comment on attachment 110117 [details] [diff] [review]
another try

ResolveStyleDataFor needs a much uglier name, since we don't want anyone to use
it.  How about ResolveStyleContextAndGetStyleData, plus a big ugly XXX comment
telling people how silly it is?

I don't see any callers for nsFrame::GetRuleNode.  Could you remove it?

In nsContextMenuInfo.cpp, I like to write the global GetStyleData as
"::GetStyleData(...)" to show that it's a global function rather than a member
function.

(There was also one thing I thought you did because you wanted to avoid
refcounting the style context outside of layout --- but there's really nothing
wrong with passing around nsStyleContext* as an opaque pointer outside of
layout, since when we deCOMify it, we'll stop having things (except for frames
/ the undisplayed map) refcount the style context.  Right?  But I can't find
that anymore, so don't worry about it.)

Did you check that there aren't any |GetStyleData| calls on frames in the
content directory?

Other than that, r+sr=dbaron.
Attachment #110117 - Flags: superreview+
Attachment #110117 - Flags: review?(dbaron)
Attachment #110117 - Flags: review+
> ResolveStyleDataFor needs a much uglier name, since we don't want anyone to use
> it.  How about ResolveStyleContextAndGetStyleData, plus a big ugly XXX comment
> telling people how silly it is?

Sure.  I thought about just using the computed style API here, but I figured it
might slow things down.  Any thoughts on that?

>I don't see any callers for nsFrame::GetRuleNode.  Could you remove it?

Done.  At one point I had that as a method on nsIFrame to be used from inspector
instead of adding GetRuleNodeForContent to nsInspectorCSSUtils.

> (There was also one thing I thought you did because you wanted to avoid
> refcounting the style context outside of layout --- but there's really nothing
> wrong with passing around nsStyleContext* as an opaque pointer outside of
> layout, since when we deCOMify it, we'll stop having things (except for frames
> / the undisplayed map) refcount the style context.  Right?  But I can't find
> that anymore, so don't worry about it.)

Right, we should be able to pass it as an opaque pointer outside of layout,
except that nsIFrame.h will have to #include nsStyleContext.h and we'll have to
export it, because of the inline functions that use mStyleContext.  Unless we
remove those methods from nsIFrame, put them on nsFrame, and pass those around
inside layout (which I think would be cleaner).  Also, we should change all
callers of nsIFrame::GetStyleContext in layout at that point to explicitly
reference the style context if they need to (I really can't imagine anyone needs
to keep a reference to it besides the frame).

> Did you check that there aren't any |GetStyleData| calls on frames in the
> content directory?

There are, but what can we do about it until we merge content and layout?  If
any of these calls are performance-critical I guess I could hack in a #define
_IMPL_NS_LAYOUT so that the calls continue to be inlined, but it would just be a
temporary measure until content and layout are merged.
> > Did you check that there aren't any |GetStyleData| calls on frames in the
> > content directory?

> There are, but what can we do about it until we merge content and layout?  If
> any of these calls are performance-critical I guess I could hack in a #define
> _IMPL_NS_LAYOUT so that the calls continue to be inlined, but it would just be a
> temporary measure until content and layout are merged.

Er, um, but nsStyleContext.cpp is right now in content, no?  Will this cause
problems on some platforms?

Other than that, the previous comment is fine with me.
Attached patch final patch (hopefully) (obsolete) — Splinter Review
This fixes all the things dbaron mentioned, and also adds #defines to the
prefix files for Mac CFM.
Attachment #110117 - Attachment is obsolete: true
Comment on attachment 110374 [details] [diff] [review]
final patch (hopefully)

carrying over r=dbaron, requesting sr=bz
Attachment #110374 - Flags: superreview?(bzbarsky)
Attachment #110374 - Flags: review+
I'll take this since I think I have a handle on how to finish it up.
Assignee: dbaron → bryner
Status: ASSIGNED → NEW
Target Milestone: Future → mozilla1.4alpha
Comment on attachment 110374 [details] [diff] [review]
final patch (hopefully)

Change things like:

> +  if (NS_FAILED(frame->GetStyleData(eStyleStruct_Visibility,
> +                                    (const nsStyleStruct*) vis)) ||

to use ::GetStyleData

classinfo could use computed style -- GetCSSPropertyValue("-moz-binding")
followed by GetStringValue will give you the URL...

in webshell/tests/viewer/nsBrowserWindow.cpp

> +        if (NS_SUCCEEDED(root->QueryInterface(NS_GET_IID(nsIFrameDebug),
> +                                              (void**) &fdbg))) {

CallQueryInterface, please.
Attached patch new patch (obsolete) — Splinter Review
addesses bz's comments, except that I didn't change the nsDOMClassInfo thing
because caillon says we get a 2% Txul win by not using the DOM computed style
interface.
Attachment #110374 - Attachment is obsolete: true
This patch was checked in.  Further work depends on bug 106161.
Status: NEW → ASSIGNED
Attachment #110374 - Flags: superreview?(bzbarsky)
Attached patch remove nsIStyleContext (obsolete) — Splinter Review
This patch finishes the job.  A couple of notes:

- StyleContexts are still refcounted, but only when necessary (hopefully). 
Most functions no longer return an addref'd style context. 
nsStyleSet::Resolve* and ProbePseudoStyleFor do return a reference, since they
may create a new style context and return the only reference to it.  The
corresponding functions on nsIPresContext pass through the owning reference to
the caller.

- This patch depends on nsRefPtr being enabled (see bug 104346).

- I removed nsIFrame::GetStyle entirely; it had no callers except for one in
commented-out code (which I changed, just to be thorough).

And, a few things that I'm not fully decided on yet:

- I avoided using the NS_ADDREF and NS_RELEASE macros for nsStyleContext raw
pointers, but there's no reason they wouldn't work.  Should I switch to using
them?
- Where should nsStyleContext.h live?
- I'd like to remove any methods that take or return an nsStyleContext from
public interfaces (such as nsIFrame) since nsStyleContext is now private to
layout. I think the right way to do that would be to move the functionality
into nsFrame, and make layout and content use nsFrame pointers instead of
nsIFrame pointers. That's a major change though, and I'd like to keep it
separate from this patch.
Attachment #112119 - Flags: review?(dbaron)
Depends on: 104346
Comment on attachment 112119 [details] [diff] [review]
remove nsIStyleContext

this crashes on the pageload test. i'll attach a new patch once i've figured
out the problem.
Attachment #112119 - Attachment is obsolete: true
Attachment #112119 - Flags: review?(dbaron)
Attached patch remove nsIStyleContext (obsolete) — Splinter Review
fixes the crash I mentioned.  The only change is ensuring that |parentContext|
isn't used uninitialized in nsFrameManager::ReResolveStyleContext().
Attachment #112258 - Flags: review?(dbaron)
Despite the vtune data mentioned at the beginning of this bug, this actually
provides no measurable performance gain in the pageload test on Mac, Linux, or
Windows. Sigh. Maybe this is no longer a hotspot?  (I'd still like to get this
in, of course)
I started to review attachment 112258 [details] [diff] [review].  Below are the comments I wrote.
However, the majority of the comments are about a single issue, and I
think it might make sense to decide what to do about that issue before I
continue reviewing the rest of the patch.  The issue is that this patch
leaves a lot of |nsStyleContext**| out parameters, and some of them are
|AddRef|ed pointers and some of them are non-|AddRef|ed pointers.  I
think the ideal would be never to use out parameters, and instead always
use |nsStyleContext*| or |already_AddRefed<nsStyleContext>| return
values.  Slightly worse, but perhaps tolerable, would be to use out
parameters to either always mean AddRefed (more consistent with current
code) or never mean AddRefed (probably easier to do).  However, I think
having a mix as currently exists will be very confusing and lead to
errors when maintaining this code later on.


Anyway, the review comments I wrote on the first chunk of the patch
(before writing the comment above) are:

nsStyleContext::FindChildWithRules should return nsStyleContext* rather
than nsresult.  Actually, it would probably make more sense to return
already_AddRefed<nsStyleContext>, since both callers either call this
or NS_NewStyleContext, which does addref.   (And, actually, it might
make sense to change NS_NewStyleContext to return
already_AddRefed<nsStyleContext> to clarify the model and avoid any
functions taking out parameters that do AddRef.)

|nsStyleContext::GetStyle| and |nsIFrame::GetStyle| can just be removed.
There aren't any callers.

|nsStyleContext::GetBorderPaddingFor| and |nsStyleContext::SetStyle|
should return |void|.

|nsStyleContext::CalcStyleDifference| should return |nsChangeHint|
(there's one caller, and it initializes the parameter to
|NS_STYLE_HINT_NONE|).

Removing the ADDREF in StyleSetImpl::ReParentStyleContext seems wrong.
The other codepaths AddRef.  Perhaps this should return
already_AddRefed<nsStyleContext> ?

in content/html/style/src/nsComputedDOMStyle.cpp, you should prefer
|dont_AddRef| to |getter_AddRefs|.  (I don't like the overloading of
|getter_AddRefs|.)

|nsInspectorCSSUtils::GetStyleContextForContent| looks like it should
return already_AddRefed<nsStyleContext>
Attachment #112258 - Flags: review?(dbaron)
I got rid of all nsStyleContext out parameters and instead use nsStyleContext*
and already_AddRefed<nsStyleContext> return values.  In the process I found a
leak and a double-release (which shouldn't have compiled; it was being called
on a nsRefPtr).
Attachment #111020 - Attachment is obsolete: true
Attachment #112258 - Attachment is obsolete: true
Comment on attachment 113967 [details] [diff] [review]
remove nsIStyleContext, part 1

dbaron, can you review this and attachment 113969 [details] [diff] [review]?
Attachment #113967 - Flags: review?(dbaron)
Attachment #113967 - Flags: superreview+
Attachment #113967 - Flags: review?(dbaron)
Attachment #113967 - Flags: review+
Attachment #113969 - Flags: superreview+
Attachment #113969 - Flags: review+
Comment on attachment 113969 [details] [diff] [review]
remove nsIStyleContext, part 2

This checkin have added 4 "may be used uninitialized" warnings on brad TBox:

+layout/html/style/src/nsCSSFrameConstructor.cpp:10646
+ `nsStyleContext*styleContext' might be used uninitialized in this function
+
+layout/html/style/src/nsCSSFrameConstructor.cpp:13388
+ `nsStyleContext*styleContext' might be used uninitialized in this function
+
+layout/html/style/src/nsCSSFrameConstructor.cpp:13687
+ `nsStyleContext*styleContext' might be used uninitialized in this function
+
+layout/html/style/src/nsCSSFrameConstructor.cpp:2869
+ `nsStyleContext*styleContext' might be used uninitialized in this function

Some of them are just a case of compiler not being smart enough, but some are
real issues, including dereferencing an uninitialized pointer in certain cases.
This has been checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
> This has been checked in.

Yeah, but the warnings are still there!
I went ahead and checked in a patch to initialize all of these to null.
Whiteboard: [whitebox]
You need to log in before you can comment on or make changes to this bug.