Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

(Blocks 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

Assignee

Description

10 years ago
Posted patch patch (obsolete) — Splinter Review
The attached patch deCOMtaminates and devirtualizes nsICSSParser.  The COM interface class nsICSSParser becomes a pointer-like wrapper class nsCSSParser, which allows us to continue hiding all the implementation details in CSSParserImpl (which still exists, but has no virtual methods anymore).  Also, responsibility for recycling parser objects now rests with nsCSSParser.cpp rather than nsCSSLoader.cpp.  The bulk of nsCSSParser.cpp did not have to change at all.  The contents of nsCSSLoader.h have been folded into nsCSSLoader.cpp, as nothing else needs to see them anymore.

From an external perspective, most uses of nsICSSParser change from

   nsCOMPtr<nsICSSParser> cssParser;
   rv = cssLoader->GetParserFor(nsnull, getter_AddRefs(cssParser));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = cssParser->ParseWhatever(...);
   NS_ENSURE_SUCCESS(rv, rv);
   cssLoader->RecycleParser(cssParser);

to

   nsCSSParser cssParser(cssLoader);
   NS_ENSURE_TRUE(cssParser, NS_ERROR_OUT_OF_MEMORY);
   rv = cssParser.ParseWhatever(...);
   NS_ENSURE_SUCCESS(rv, rv);

If you don't have a nsICSSLoader object handy you can just construct the parser object with no arguments, and it'll still participate in the recycling scheme.
Attachment #407432 - Flags: superreview?(bzbarsky)
Attachment #407432 - Flags: review?(dbaron)
Assignee

Updated

10 years ago
OS: Linux → All
Hardware: x86 → All
Version: 1.9.0 Branch → Trunk
Hmm....  This makes me wonder.  Why are CSSParserImpls heap-allocated anyway?  The recycling scheme is just to prevent having to hit malloc for them all over, right?  But do they actually need to live on the heap at all?
>+    nsAutoString styleAttr(NS_LITERAL_STRING("line-height:normal;font:"));
>+    styleAttr.Append(aFont);

Don't those properties need to be in the opposite order?  If we didn't have a test that caught this, add one?

> nsSVGElement::UpdateContentStyleRule()

The first null-check of |declaration| makes no sense.  It's always null there, no?  Also, this is a logic change: now we create the declaration even if we have no style-mapped attrs, as long as we have _some_ attrs.  That seems wrong.

You don't need the cssLoader local here anymore, right?

Why is the anonymous namespace around the CSSParserImpl class decl needed (or desired)?  This might just be me not being familiar enough with namespaces, but I'd like to understand what that does.

Annotate nsCSSParser with NS_STACK_CLASS?  Then you might not need the comment about not using pointers either.

> +  // XXX This really ought to be operator bool.

Why not just make it so?

Why make mImpl void* instead of CSSParserImpl* (with a |class CSSParserImpl| or something) and avoiding the casts?

The rest of the patch looks fine (and quite nice, actually).  Longer-term, we might want to change things so that GetCSSParsingEnvironment goes away somehow.  Need to think about it a bit.
Assignee

Comment 3

10 years ago
(In reply to comment #1)
> Hmm....  This makes me wonder.  Why are CSSParserImpls heap-allocated anyway? 
> The recycling scheme is just to prevent having to hit malloc for them all over,
> right?  But do they actually need to live on the heap at all?

I don't think they do, and making them stack objects would get rid of a whole bunch more wacky initialization code.  I didn't do it partially because that would have made this patch have to touch a lot more of nsCSSParser.cpp, and partially because C++ doesn't give us a nice way to do that without exposing all the internal parser methods in nsCSSParser.h.  As one of the go-to guys for new CSS syntax, I am unenthusiastic about having to recompile everything that uses the parser interface when I just want to mess with the internals.

Neither of these is to say it shouldn't happen, just maybe not here.

(In reply to comment #2)
> >+    nsAutoString styleAttr(NS_LITERAL_STRING("line-height:normal;font:"));
> >+    styleAttr.Append(aFont);
> 
> Don't those properties need to be in the opposite order?  If we didn't have a
> test that caught this, add one?

Ugh, yes, you're right; I blithely assumed font: couldn't override line-height:.  Will fix.

> > nsSVGElement::UpdateContentStyleRule()
> 
> The first null-check of |declaration| makes no sense.  It's always null there,
> no?  Also, this is a logic change: now we create the declaration even if we
> have no style-mapped attrs, as long as we have _some_ attrs.  That seems wrong.

This is just me being clumsy and overzealous about simplifying control flow.  I missed the conditional at the beginning of the loop.  Will fix.

> You don't need the cssLoader local here anymore, right?

I guess I could just do  "nsCSSParser parser(doc->CSSLoader())".

> Why is the anonymous namespace around the CSSParserImpl class decl needed (or
> desired)?  This might just be me not being familiar enough with namespaces, but
> I'd like to understand what that does.

The effect is to make all of the CSSParserImpl method symbols file-local.

> Annotate nsCSSParser with NS_STACK_CLASS?  Then you might not need the comment
> about not using pointers either.

Ok.

> > +  // XXX This really ought to be operator bool.
> Why not just make it so?

I was under the impression 'bool' could not be used at all in our code?

> Why make mImpl void* instead of CSSParserImpl* (with a |class CSSParserImpl| or
> something) and avoiding the casts?

If you do that, then, unfortunately, you can't put CSSParserImpl in the anonymous namespace.

> The rest of the patch looks fine (and quite nice, actually).  Longer-term, we
> might want to change things so that GetCSSParsingEnvironment goes away somehow.
>  Need to think about it a bit.

I wouldn't be surprised if CSSLoader could get the same treatment, as a step toward that...
> Neither of these is to say it shouldn't happen, just maybe not here.

Sure.  I wasn't saying it needs to happen here, and I don't think it should.  Followup, though.

I'd be fine with having a stack object with the public cssparser api which then uses another stack object which is the real parser.  The header for the former would not include the header for the latter, but the impl would.

> I guess I could just do  "nsCSSParser parser(doc->CSSLoader())".

Yep.

> The effect is to make all of the CSSParserImpl method symbols file-local.

So just a way to make sure people don't call them by hand or something?  Might be worth documenting....

> I was under the impression 'bool' could not be used at all in our code?

I believe we've rescinded that now that we're no longer compiling with VisualAge on OS/2.  We still can't use it in places where we have PRBool ABI, but for internal code we can use it (and spidermonkey, e.g. has been using it heavily).

> If you do that, then, unfortunately, you can't put CSSParserImpl in the
> anonymous namespace.

Ah, ok.  Again, comment that?

> I wouldn't be surprised if CSSLoader could get the same treatment

CSSLoader is used across module boundaries and has nontrivial lifetime (unlike the parser, which can always go away once you pop it off the stack).  So might be harder there.
Assignee

Comment 5

10 years ago
(In reply to comment #4)
> 
> I'd be fine with having a stack object with the public cssparser api which then
> uses another stack object which is the real parser.  The header for the former
> would not include the header for the latter, but the impl would.

I'll look into that.  Or I suppose I could have an implementation subclass with no data members, just methods (since the methods are the main thing I want to keep out of the header file -- they change a lot more often than the data members).

> > The effect is to make all of the CSSParserImpl method symbols file-local.
> 
> So just a way to make sure people don't call them by hand or something?  Might
> be worth documenting....

Not exactly; it's more about making less work for the linker.  And I think it's something that should go in the style guide, if anything ("every class that is fully declared in a .cpp file and never mentioned in any .h file can and should be put into the anonymous namespace, to make all its member methods file-local.  consider using void* rather than type* to point to implementation objects hidden in a .cpp file so that they can be in the anonymous namespace").

> I believe we've rescinded that now that we're no longer compiling with
> VisualAge on OS/2.  We still can't use it in places where we have PRBool ABI,
> but for internal code we can use it (and spidermonkey, e.g. has been using it
> heavily).

Ok, changed.

> Ah, ok.  Again, comment that?

Done.

> CSSLoader is used across module boundaries and has nontrivial lifetime (unlike
> the parser, which can always go away once you pop it off the stack).  So might
> be harder there.

Yah, it might need to be a refcounted non-COM object rather than a stack object.  We don't seem to have handy macros for implementing AddRef/Release semantics on non-COM objects, or have I missed something?

Coming back to ...

> >+    nsAutoString styleAttr(NS_LITERAL_STRING("line-height:normal;font:"));
> >+    styleAttr.Append(aFont);
> 
> Don't those properties need to be in the opposite order?  If we didn't have a
> test that caught this, add one?

... this, I fixed the code, but I can't find any way to catch it with a test, because there's no way to draw more than one line of text at a time in canvas.  HTML5 does say that you're not supposed to get the line-height back out when you _read_ .font, even if you set it, but our implementation doesn't do that regardless of the order.  Thoughts?
Assignee

Comment 6

10 years ago
Attachment #407432 - Attachment is obsolete: true
Attachment #408094 - Flags: superreview?(dbaron)
Attachment #408094 - Flags: review?(bzbarsky)
Attachment #407432 - Flags: superreview?(bzbarsky)
Attachment #407432 - Flags: review?(dbaron)
> Thoughts?

Maybe check with dbaron to see whether this can affect something other than the .font getter (which we should consider just fixing)?
I was rewriting the ctx.font getter and setter code in bug 508452, but I got busy with other things.
Assignee

Comment 9

10 years ago
Posted patch CSS parser interface cleanup (obsolete) — Splinter Review
Here's another patch, on top of the current deCOM one, that gets rid of exposed CSSParser objects altogether.  I realized that all current uses create the parser, call one Parse* method on it, and throw it away - so why not make that usage explicit?  The new exposed interface is a handful of free functions in the mozilla::CSS namespace.  There's still a CSSParserImpl object but it's entirely private to nsCSSParser.cpp now.

Further API cleanup is possible, I'm sure (we should take a hard look at what the callers of these methods do with the nsresult return codes, for instance), but I thought this was invasive enough for one patch.
Attachment #411007 - Flags: superreview?(dbaron)
Attachment #411007 - Flags: review?(bzbarsky)
Duplicate of this bug: 516200
Attachment #408094 - Flags: review?(bzbarsky) → review+
Before I start looking at the details of the interface cleanup, I have a question.  In at least some places (e.g. ParseSelectorString) it looks like the new API doesn't pass in the CSSLoader, which means we no longer set child loader and the quirks mode.  The former is ok for ParseSelectorString; for the latter it seems to be relying on the fact that we have no quirks in selector-parsing.  That should at least be documented in the API, or something.  Similar for ParseMediaList, at least.  Are there other things that make such assumptions?
Assignee

Comment 12

10 years ago
Posted patch deCOM patch v3 (rediffed) (obsolete) — Splinter Review
The deCOM patch needed to be merged with a bunch of changes that have gone into mainline.
Attachment #408094 - Attachment is obsolete: true
Attachment #421184 - Flags: superreview?(dbaron)
Attachment #421184 - Flags: review+
Attachment #408094 - Flags: superreview?(dbaron)
Assignee

Comment 13

10 years ago
This needed rediffing too, but the interesting change is much better comments in nsCSSParser.h -- I hope they address your question in comment 11, Boris.  (Summary: there are only three CSS quirks, and we [dbaron and I] have no plans to add others; the interfaces that don't pass a loader either cannot be affected by quirks (media-list, selector-list) or didn't want quirks behavior anyway (color).
Attachment #421184 - Attachment is obsolete: true
Attachment #421185 - Flags: superreview?(dbaron)
Attachment #421185 - Flags: review?(bzbarsky)
Attachment #421184 - Flags: superreview?(dbaron)
Assignee

Updated

10 years ago
Attachment #411007 - Attachment is obsolete: true
Attachment #411007 - Flags: superreview?(dbaron)
Attachment #411007 - Flags: review?(bzbarsky)
Assignee

Comment 14

10 years ago
Comment on attachment 411007 [details] [diff] [review]
CSS parser interface cleanup

Doh, I marked the wrong thing obsolete.
Assignee

Updated

10 years ago
Attachment #421184 - Attachment is obsolete: false
Assignee

Updated

10 years ago
Attachment #421184 - Flags: superreview?(dbaron)
Comment on attachment 421185 [details] [diff] [review]
interface patch v2 (rediffed, better commentary)

>+++ b/layout/style/nsCSSParser.cpp
> typedef void (* RuleAppendFunc) (nsICSSRule* aRule, void* aData);
....
>+typedef void (* RuleAppendFunc) (nsICSSRule* aRule, void* aData);

Why do you need to add that?

> CSSParserImpl::SetSVGMode(PRBool aSVGMode)

This doesn't look like it would compile if --disable-svg, since the scanner's SetSVGMode is conditioned on that ifdef.

Why the various changes from nsSubstring to nsAString?

r=bzbarsky with those nits.  Sorry for the lag here...
Attachment #421185 - Flags: review?(bzbarsky) → review+
Assignee

Comment 16

10 years ago
(In reply to comment #15)
> (From update of attachment 421185 [details] [diff] [review])
> >+++ b/layout/style/nsCSSParser.cpp
> > typedef void (* RuleAppendFunc) (nsICSSRule* aRule, void* aData);
> ....
> >+typedef void (* RuleAppendFunc) (nsICSSRule* aRule, void* aData);
> 
> Why do you need to add that?

That's a merge error.  In the original iteration of these patches, I moved that typedef from the header file in part 2 rather than part 1.  When revising I realized there were no exposed users so it might as well be moved in part 1, and then I forgot to take it out of part 2 as well.  And apparently C++ is perfectly fine with duplicate typedefs (unlike C) so the compiler didn't care.

> > CSSParserImpl::SetSVGMode(PRBool aSVGMode)
>
> This doesn't look like it would compile if --disable-svg, since the scanner's
> SetSVGMode is conditioned on that ifdef.

Does anyone ever actually *use* --disable-svg?  Will fix.

> Why the various changes from nsSubstring to nsAString?

Some of the exposed functions took an nsSubstring& and some took an nsAString&.  I thought it would be good to make them consistent.  I checked the definitions (nsStringFwd.h) and noticed that nsSubstring is just a typedef for nsAString and is labeled "backward compatibility", so I figured might as well just search-and-replace nsSubstring with nsAString in the whole file.

> r=bzbarsky with those nits.  Sorry for the lag here...

No worries.  (I'm still waiting for dbaron on part 1, after all.)
> Does anyone ever actually *use* --disable-svg?

Probably yes. I think we should remove the option, but not piecemeal.  ;)
Assignee

Comment 18

10 years ago
Attachment #421185 - Attachment is obsolete: true
Attachment #423055 - Flags: superreview?(dbaron)
Attachment #423055 - Flags: review+
Attachment #421185 - Flags: superreview?(dbaron)
Assignee

Updated

10 years ago
Blocks: 541496
Assignee

Comment 19

10 years ago
I noticed only after posting that last revision that CSSParserImpl::Reset() is now never called, so this revision removes it.  No other changes.
Attachment #423055 - Attachment is obsolete: true
Attachment #423064 - Flags: superreview?(dbaron)
Attachment #423064 - Flags: review+
Attachment #423055 - Flags: superreview?(dbaron)
Assignee

Comment 20

10 years ago
This revision changes some names:

 - 'parser' instead of 'impl' for the CSSParserImpl objects created in the interface functions at the bottom of the file
 - 'ParseSheet' instead of 'Parse' for the main parser entrypoint

marks CSSParserImpl a stack-only class, and removes a confusing comment.  (I found myself making all these changes in a later patch in the series & decided maybe they belonged here instead.)
Attachment #423064 - Attachment is obsolete: true
Attachment #423461 - Flags: superreview?(dbaron)
Attachment #423461 - Flags: review+
Attachment #423064 - Flags: superreview?(dbaron)
Assignee

Updated

10 years ago
Depends on: 528096
Assignee

Comment 21

10 years ago
It turns out that the "interface patch" adds substantial overhead.  With a modified version of bz's microbenchmark from bug 498559, which I'll post shortly, I get these numbers (Linux/x86-64 optimized build).  Times are microseconds per single call to the parser API.

yesterday's trunk
      ParseProperty:    1.25 us
      ParseColorString: 0.67 us

+ deCOM
      ParseProperty:    1.05 us
      ParseColorString: 0.46 us

+ interface
      ParseProperty:    3.44 us
      ParseColorString: 2.11 us

+ |mData| created only when needed
      ParseProperty:    2.65 us
      ParseColorString: 1.45 us

+ |mTempData| created only when needed
      ParseProperty:    2.67 us
      ParseColorString: 0.58 us

I think most of the overhead is because the interface patch removed CSSParserImpl object recycling; instead we create a new one every time.  nsCSSExpandedDataBlock is large and its constructor does a lot of work, and there are two of them embedded in CSSParserImpl.

It's relatively easy to avoid creating either of them for ParseColorString(), and as you can see, a patch that does that gets rid of almost all the new overhead (though not all - still investigating that).  Unfortunately, as-is that patch creates a new expanded data block for *every property*, and another one for every declaration block, which is why the ParseProperty overhead hasn't gone back down as much.

I think maybe I should withdraw the "interface patch" for now, re-file it in a new bug that depends on the "get rid of expanded data blocks" patch (which I never actually filed a bug for), suspend work on the rest of this patch series and switch over to working on that till it's done.  That'll make getting rid of this overhead much easier.
Assignee

Comment 22

10 years ago
Posted file microbenchmark
Assignee

Updated

10 years ago
Attachment #423461 - Flags: superreview?(dbaron)
Attachment #423461 - Flags: review-
Attachment #423461 - Flags: review+
Can you just put the parser recycling back in to this patch and then move forward on the series of 4 patches that you already have?
Assignee

Comment 24

10 years ago
Unfortunately, the subsequent 4 patches were developed on the theory that two-phase initialization of CSSParser and CSSScanner was going away.  Putting parser recycling back would mean a whole lot of fiddly merge work to restore two-phase init.

It occurs to me, though, that we could recycle nsCSSExpandedDataBlock objects instead.  The initialization story is a lot simpler there.  Stay tuned?
Assignee

Comment 25

10 years ago
Recycling blocks helped a lot, but the overall results are still not as good as I had hoped.  Here's how each stage in my current patchset does on the microbenchmark:

deCOM
      ParseProperty:    1.05 us
      ParseColorString: 0.46 us

recycle blocks
      ParseProperty:    1.17 us
      ParseColorString: 0.47 us

interface
      ParseProperty:    1.29 us
      ParseColorString: 0.56 us

no streams
      ParseProperty:    1.29 us
      ParseColorString: 0.55 us

split error reporter
      ParseProperty:    1.17 us
      ParseColorString: 0.78 us

clean up init
      ParseProperty:    1.04 us
      ParseColorString: 0.69 us

simpler error api
      ParseProperty:    1.03 us
      ParseColorString: 0.69 us

no pushback buffer
      ParseProperty:    1.21 us
      ParseColorString: 0.69 us

lazy error positions
      ParseProperty:    1.13 us
      ParseColorString: 0.65 us

I'm particularly disappointed in the last two patches making things worse and then not much better.
Is that still using the PR_strtod-backed ParseNumber?
Assignee

Comment 27

10 years ago
No, I took that back out.
Assignee

Updated

10 years ago
Blocks: 544112
Assignee

Updated

10 years ago
Blocks: deCOM
(In reply to comment #25)
> Recycling blocks helped a lot, but the overall results are still not as good as
> I had hoped.  Here's how each stage in my current patchset does on the
> microbenchmark:

I'm having trouble understanding why the patch 'interface' is a slowdown.  Wouldn't the performance hit from losing the recycling show up in 'deCOM'?  Or am I misunderstanding the presentation of the data?
Assignee

Comment 29

10 years ago
The 'deCOM' patch doesn't lose the recycling, it just moves it from nsCSSLoader to nsCSSParser.  It was 'interface' that (in its original version) abandoned recycling in favor of stack objects.
Comment on attachment 421184 [details] [diff] [review]
deCOM patch v3 (rediffed)

sr=dbaron
Attachment #421184 - Flags: superreview?(dbaron) → superreview+
Assignee

Comment 31

10 years ago
Attachment #423461 - Attachment is obsolete: true
Assignee

Comment 32

9 years ago
http://hg.mozilla.org/mozilla-central/rev/f6beeb315747
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee

Updated

9 years ago
Attachment #421184 - Attachment is obsolete: true

Comment 33

9 years ago
Is this gFreeList thing MT safe? Doesn't it need to be so? Otherwise the RecyclingAllocator may help (also for the nsCSSExpandedDataBlock objects?)
Assignee

Comment 34

9 years ago
CSS parsing always happens on the main thread AFAICT.  The free list is going away soon, so I'm not going to bother switching to RecyclingAllocator.

Comment 35

9 years ago
Other question: in nsCSSScanner init/destructor there is:
272   MOZ_COUNT_CTOR(nsCSSScanner);
Is still needed, as nsCSSScanner is fixed member of nsCSSParser?

Comment 36

9 years ago
Another optimization would be to not released buffers in nsCSSScanner when it is recycled: nsCSSScanner::Close() frees mFileName,mError,mPushback
and nsCSSScanner::Close() is only called when the scanner is released for re-use.
These three buffers are generally very small, but releasing and reallocating them again is costly. Whilst nsCSSParser is reused, why not also reuse these as well?
(In reply to comment #35)
> Other question: in nsCSSScanner init/destructor there is:
> 272   MOZ_COUNT_CTOR(nsCSSScanner);
> Is still needed, as nsCSSScanner is fixed member of nsCSSParser?

It doesn't hurt, so I think we're better off keeping it.
Hmmm.  The changes to nsCanvasRenderingContext2D.cpp in http://hg.mozilla.org/mozilla-central/rev/f6beeb315747 are substantively incorrect, since they allow a bunch of things for ctx.font that should not be allowed.  Hopefully I'll wipe those changes away when I land bug 508452, though.
Assignee

Comment 39

9 years ago
To the extent that it's wrong, I claim it was already wrong (I assume you're talking about things like allowing 'inherit' and 'initial'...)
No, I'm talking about allowing things like "1em serif; background: green; margin: 10px".
You need to log in before you can comment on or make changes to this bug.