Open Bug 196843 Opened 21 years ago Updated 5 months ago

Fastload CSS style sheets

Categories

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

x86
All
defect

Tracking

()

People

(Reporter: janv, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: mobile, perf, Whiteboard: startup [ts])

Attachments

(2 files, 4 obsolete files)

 
Priority: -- → P3
Target Milestone: --- → mozilla1.6alpha
This should wait until after I'm done bug 125246.
Component: XP Apps → Style System
Depends on: 125246
I must say that I have no concrete plans to implement it in near future.
Anyway, it would be very nice to have it implemented :)

Btw, would it be difficult to implement serializing and deserializing
methods for CSS style sheets ?
I'm asking since I'm not an expert on the style system.
(In reply to comment #1)
> This should wait until after I'm done bug 125246.

Looks like that one is done. ;-)
Sorry, I don't plan to work on this in near future.
In that case, should the milestone be bumped, or the bug just marked as WONTFIX?
Target Milestone: mozilla1.6alpha → Future
*** Bug 175410 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b4?
Flags: blocking1.8b4? → blocking1.8b4-
Blocks: 203448
Assignee: Jan.Varga → bryner
Target Milestone: Future → mozilla1.9alpha
Attached patch patch (obsolete) — Splinter Review
This seems to give about a 3-4% startup time win for me, though I haven't really been able to get consistent test times (my estimate is based on Quantify data).  There's a lot of room for further optimization by improving the fastload architecutre (for example, moving away from architecture independence so that larger chunks of memory can be read and written).
Attachment #200933 - Flags: superreview?(brendan)
Attachment #200933 - Flags: review?(dbaron)
Attached patch patch (-w) (obsolete) — Splinter Review
It'll take me at least until next week to get to this review.

I'd like Boris to look at least at the LoadImportedSheet-related stuff in this patch.
So if nothing else, the child sheet thing means that Read() might not be sync, effectively.  So setting mComplete = PR_TRUE at the end of Read() is probably wrong, since child sheets may still be loading.
Comment on attachment 200933 [details] [diff] [review]
patch

I'm also not going to be able to really look at this until at least next week, possibly later...
Attachment #200933 - Flags: review?
Attached patch a couple of small fixes (obsolete) — Splinter Review
This patch makes us only set mComplete if all of our child sheets are complete at the end of Read().  Also, I fixed a nsCSSLoader::HandleLoadEvent to not call CacheStyleSheet if the stylesheet was already complete (since in that case it has presumably already been cached).  This makes it so that we don't serialize the same sheet twice.
Attachment #200933 - Attachment is obsolete: true
Attachment #200935 - Attachment is obsolete: true
Attachment #201056 - Flags: superreview?(brendan)
Attachment #201056 - Flags: review?(dbaron)
Attachment #200933 - Flags: superreview?(brendan)
Attachment #200933 - Flags: review?(dbaron)
Attachment #200933 - Flags: review?
Attached patch diff -w (obsolete) — Splinter Review
Attachment #201056 - Flags: review?(bzbarsky)
Comments on just nsCSSDataBlock and nsCSSDeclaration:

nsCSSDataBlock.cpp:

+    // TODO: this is a fairly inefficient way to read/write the datablock,
+    // since it results in many small reads/writes.  This could be improved
+    // by coalescing consecutive "primitive" values and just memcpy'ing the
+    // entire block (where a primitive value is one that does not use
+    // pointer storage in the nsCSSValue).  Before this can happen, we need to
+    // transition to an architecture-dependent fastload file format, with
+    // appropriate sanity checking to make sure it's compatible.

I'm not sure I'd put this in as a "TODO", for a bunch of reasons:
 * many of the types have pointers
 * being architecture-independent is valuable

+        rv = aStream->Read32(NS_REINTERPRET_CAST(PRUint32*, cursor));

For a start, you shouldn't be depending on PropertyAtCursor being a cast
with no offset, so you should call it.  Second, you can't depend on an
enum type being 32 bits (it could easily be 64), so you need to use
assignment rather than casting.  So this should really be something
like:

PRUint32 prop32;
rv = aStream->Read32(&prop32);
PropertyAtCursor(cursor) = prop32;

+            case eCSSType_ValueList: {
+                CDBPointerStorage* storage =
+                    NS_REINTERPRET_CAST(CDBPointerStorage*, cursor);
+                rv = NS_ReadLinkedList(aStream,
+                                       NS_REINTERPRET_CAST(nsCSSValueList**,
+                                                           &storage->value));

This shouldn't use CDBPointerStorage (which should be used as little as
possible, and really only exists at all to ensure alignment), so it can
just be:

rv = NS_ReadLinkedList(aStream, &ValueListAtCursor(cursor));

and likewise for the CounterData and Quotes cases.


nsCSSDeclaration.cpp:

Please add a comment in Write that we really shouldn't ever be getting
into a state where we're writing a declaration that has a null mData
since the code *should* protect against getting to that point, but it
currently doesn't.
(In reply to comment #14)
>  * being architecture-independent is valuable

I'm not sure I agree, but darin and brendan are working on that, so I'll leave it out of this code comment.
nsChromeProtocolHandler.cpp:

  Do you really want to make it that only XUL files with a .xul file
  extension are fastloaded?  Should we really be basing things on
  extensions?

nsXMLNameSpaceMap.cpp:

  Your Read method here should use NS_ReadAtom for symmetry (and because
  it'll make it easier to fix some nasty UTF-8 vs. ASCII (vs. UTF-16)
  issues in the nsIBinary* interfaces and elsewhere that I'll get to
  later, or prevent nasty bugs if NS_ReadAtom and NS_WriteAtom are ever
  changed).  Then your new constructor can take an atom instead of a
  string.

nsIXULPrototypeCache.h:

  I don't understand this callback thing just looking at it.  Hopefully
  I will later, but it should be better commented.  For example, can it
  be null?

  ... well, after looking at it, there only seems to be one callback
  ever used, and I'm not sure what it would mean to do otherwise.  Is
  the purpose so that there can be multiple types of style sheets?  If
  so, say so somewhere.

  Also maybe rename aData to aClosure so it's clearer that it's
  for passing to aCallback.

nsXULPrototypeCache.cpp:

  +    if (!gXULCacheLog) {
  +        gXULCacheLog = PR_NewLogModule("XULCache");
  +    }

  Should this be #ifdef-ed?  (Along with the other uses?)
  gXULCacheLog's definition is ifdef-ed, so there's clearly something
  wrong.

  I'll leave looking over GetStyleSheet and WriteStyleSheet carefully to
  brendan.

So now I'm up to nsCSSLoader.cpp
nsCSSLoader.cpp:

  We have a bunch of identical functions called IsChromeURI across the
  tree.  I have a patch in a tree that consolidates them.  If you're
  changing the semantics of one of them, please rename it to
  IsChromeOrResourceURI or something like that.

  Would your callback mechanism (DeserializeSheetCB) be better
  implemented as an interface (nsIStyleSheetLoader, a base class of
  nsICSSLoader) if it's intended to be something that allows multiple
  types of stylesheets to be fastloaded?  Then you could drop two
  parameters to one, and be a little more in-style.  (And if that is the
  intent, why is one of the callback parameters nsICSSStyleSheet?  And
  if it's not, why have a callback at all?)

  Why did you pull CacheStyleSheet out of SheetComplete?  So far, it
  seems that what you've changed is:
   * the order that CacheStyleSheet and the very last bit of
     SheetComplete happen
   * whether CacheStyleSheet happens when wasComplete is true in
     HandleLoadEvent
  Which was the intent (I'm guessing the latter, because of the change
  within CacheStyleSheet), is the other safe, and is there an easier way
  (in particular, I don't mind splitting things into functions, but
  having to call from three places instead of one is annoying)?

  -    if (data->mParentData &&
  -        --(data->mParentData->mPendingChildren) == 0 &&
  -        mParsingDatas.IndexOf(data->mParentData) == -1) {
  +    SheetLoadData* parentData = data->mParentData;
  +    if (parentData) {
  +      if (--(parentData->mPendingChildren) == 0 &&
  +          mParsingDatas.IndexOf(parentData) == -1) {

  The new variable here is nice, but why split the if into two ifs?
Also, speaking of IsChromeURI, have you checked that there aren't jar: URIs getting through to the CSS loader as the URIs of CSS stylesheets (instead of the equivalent chrome: URIs)?  I may have fixed the last of those while working on bug 226791 (which is also the patch I was referring to that consolidated IsChromeURI, but that part hasn't landed), but I'm not sure.
Er, the patch and most of the work were on bug 252703, not bug 226791.
To answer your questions about the callback stuff in nsIXULPrototypeCache -- I originally had this as just a method on nsICSSLoader, but it didn't seem good to expose it this way when it doesn't give you a ready-to-use sheet (PrepareSheet has not been called on it).  This way allows the CSSLoader to give the XULPrototypeCache a function to use, without otherwise exposing it.  I hadn't really thought to use it for fastloading other types of nsIStyleSheet objects.
nsCSSRules.cpp:

  CSSImportRuleImpl needs to write and read mMedia (like CSSMediaRule)

  nsCSSGroupRule::Read should just NS_ASSERTION that the rule type isn't
  IMPORT_RULE, since @import rules aren't allowed there

  nsCSSDocumentRule::URL::Read can't NS_REINTERPRET_CAST to read func,
  since enums need not be 32-bit.  You need to read into a 32-bit
  variable and then assign to the enum.

nsIBinaryInputStream.idl and nsIBinaryOutputStream.idl (in part):

  NS_ReadAtom and NS_WriteAtom rely on something that doesn't seem to me
  to be true:  that you can't call NS_NewAtom("").  I think you need
  NS_ReadOptionalAtom and NS_WriteOptionalAtom, and NS_ReadAtom and
  NS_WriteAtom can't null-check.

nsCSSStyleRule.cpp:

  You have comments:

  +  // The list owner is in charge of reading and creating all list nodes
  +  // The list owner is in charge of walking the list to write all nodes

  in many of the Read methods for NS_ReadLinkedList or Write methods for
  NS_WriteLinkedList (but starting in this file, not in any of the prior
  files in the patch).  Could you either remove them or add them to the
  rest?  (And it might be good if they mentioned NS_ReadLinkedList and
  NS_WriteLinkedList if they're there.)

  nsAtomStringList::Read and Write seem to turn a null mString into an
  empty (non-null) mString.  (The string methods on nsIBinary*Stream
  really need improvement (for nullness-consistency, encoding issues,
  and general paralellism).)

  I'm up to nsAttrSelector
nsCSSStyleRule.cpp, continued:

  In nsAttrSelector::Write, you should perhaps cast mCaseSensitive to
  PRUint16 before shifting it, i.e., instead of
  +  rv = aStream->Write16(mFunction | (mCaseSensitive << 8));
  you should write
  +  rv = aStream->Write16(mFunction | (PRUint16(mCaseSensitive) << 8));
  (I think the standard says this isn't needed, but it is a bit unclear
  since it says "integral promotions are performed" when integral
  promotions are defined as a list of "can be converted to" rules.)

  In nsCSSSelector::Read, I think you'll need a cast for some platforms
  for the Read16 call to read in a PRUnichar.

  +  CSSStyleRuleImpl() : mImportantRule(nsnull), mDOMRule(nsnull) {}
  Could you begin the initializers with "nsCSSRule()," to follow
  existing conventions in this code?

  There may be a need to fastload mDOMRule at some point in the future
  (since other things, primarily data structures containing variables in
  scripting languages, but potentially other things, could have a
  reference to it, but you're not fastloading any of them now).  It's
  worth at least putting a nasty comment in CSSStyleRuleImpl::Read and
  Write, if not actually doing it (although I don't really expect us to
  fastload any of those things).

  The same goes for mImportantRule if we were ever to fastload the rule
  tree (which is actually probably more realistic than something holding
  mDOMRule), and it's also easier to fastload.

  CSSStyleRuleImpl::Read needs to set mDeclaration to null if
  haveDeclaration is false.  (Hopefully we're never writing any rules
  without a declaration, but given that you're handling that case, you
  should handle it correctly.)

  CSSStyleRuleImpl::Read should call mDeclaration->RuleAbort() instead
  of delete mDeclaration in the failure case.  It should also set
  mDeclaration to null after doing this.

  Also, it's worth commenting that a pattern you use in many of the
  static Read methods (used by NS_ReadLinkedList) is probably suboptimal
  for codesize, at least on older compilers.  You currently construct
  the object you're reading at the start of the method and then put in a
  lot of early returns, when in many cases there's nothing preventing
  you from waiting to construct the main object until the end of the
  method.  (That could also avoid the need for some of the extra
  constructors you had to add, although it sometimes might add the need
  for additional reference counting, although .swap() should avoid that
  problem.)  That said, I don't think it's worth going through this
  patch and fixing all of them, but it's worth considering in the
  future.

nsIMediaList.h (partly):
  You should have a comment before Read saying that the caller is also
  responsible for calling SetStyleSheet.

nsCSSRules.cpp (addition):
  nsCSSMediaRule::Read should have a comment that the callers
  SetStyleSheet call on the media rule will do the required
  SetStyleSheet call on mMedia.

  (But don't do anything for nsCSSImportRule, where you may need to add
  media list handling, since the sheet and the import rule shares the
  media list with the sheet, and the sheet manages the SetStyleSheet.
  More on that later...)

nsCSSRules.h (addition):
  All the Read methods should have a comment that the caller must call
  SetStyleSheet and, if needed, SetParentRule.

nsCSSStyleSheet.cpp:

  You added an include of "nsCSSDeclaration.h".  Why is this needed?
  nsCSSStyleSheet really shouldn't need to know anything about
  nsCSSDeclaration, and I don't see any obvious reason that it's needed.
  (I'd prefer to leave it out so it would be a red flag in any later
  review that does add such a dependency.)

  In nsMediaList::Read, it's worth mentioning that it's backwards so
  the array only gets expanded once.

  I'm up to nsCSSStyleSheetInner::Read
nsCSSStyleRule.cpp (additional comment):

  In nsCSSStyleRuleImpl::Read and Write, it's worth commenting that
  mDeclaration doesn't need to use ReadObject/WriteObject because
  they're never actually shared, and the only reason they're still
  refcounted is for bug 209575 (see bug 209433).

nsCSSStyleSheet.cpp, continued:

  nsCSSStyleSheetInner::Read and Write don't do anything with mComplete.
  They probably should, especially if you do what I suggest two comments
  down.

  nsCSSStyleSheet::Read and Write don't do anything with mMedia.  They
  should.
  nsCSSMediaRule::Read, nsCSSImportRule::Read, and nsCSSStyleSheet
  should all use WriteObject/ReadObject when dealing with nsMediaList,
  especially since nsCSSImportRule and nsCSSStyleSheet actually do rely
  on sharing the object.  (Alternatively, you might be able to make
  things work right by skipping the handling on nsCSSImportRule and
  relying on the stylesheet to set its media list on the import rule.
  But it seems less fragile to rely on WriteObject/ReadObject.)
  Note that the media list handling is currently missing from both
  nsCSSImportRule and nsCSSStyleSheet.

  I notice you decided to fill in the sheet pointers in nsCSSStyleSheet
  by triggering new loads from @import rules.  An alternative, perhaps
  more consistent with the fastload design, would be to use ReadObject /
  WriteObject for all of these pointers (generally
  NS_ReadOptionalObject, etc.).  This seems less risky to me, actually,
  since there's less worrying about whether you should or should not
  write out each member -- you just write out everything.  (For example,
  should you write nsCSSStyleSheetInner::mComplete?).  This would mean
  pretty much rewriting nsCSSStyleSheet's Read and Write methods, giving
  CSS stylesheets a CID if they don't already have one, removing
  LoadImportedSheet and its callers, removing sCurrentLoader, removing
  nsICSSImportRule::GetMediaList, and perhaps using
  ReadObject/WriteObject in a few other places, although I haven't
  noticed any.

  NS_ReadCSSRule should at least have nasty comments that if we ever
  fastload the rule tree, we need to make it use ReadObject and
  WriteObject rather than creating new objects unconditionally.
  (See also my comment on fastloading mDOMRule and mImportantRule in
  nsCSSStyleRule.cpp.  I guess given the CID pain of ReadObject, it's
  probably not worth doing either of those, but do add comments with
  warnings.)

  The default case in NS_ReadCSSRule should assert in addition to
  failing.  (There's a very old patch lying around to implement @page.)

nsCSSValue.cpp:

  nsCSSValue contains some reference counted data structures that really
  can be shared between values.  Based on what you're currently
  fastloading, however, you'll never hit multiple copies of the same
  value.  Also, I *think* the sharing is currently only used for
  optimization.  However, you should nevertheless have a nasty comment
  in nsCSSValue::Read about this because it has the potential to read
  different data structures than the ones that were written.

  The URL section of nsCSSValue::Write should either:
   (1) contain
       NS_ASSERTION(mUnit != eCSSUnit_Image || mValue.mURL ==
         NS_STATIC_CAST(URL*, mValue.mImage), "bad union usage");
   or
   (2) instead of
   +    URL* url = mValue.mURL;
   say:
   +    URL* url = mUnit == eCSSUnit_Image ? mValue.mImage : mValue.mURL;
   which the compiler should be able to optimize to your current code as
   long as the assertion wouldn't fire.  I prefer the latter, because
   it's more portable and consistent with the way we handle Image
   deriving from URL in other parts of the code.

  In nsCSSValue::Read and Write, the mString value on URL values is not
  optional, it's required.  Don't use the "Optional" functions.

nsICSSLoader.h:

  I don't see why CacheStyleSheet is being added to the interface and
  made virtual when all the calls are within nsCSSLoader.  (See my
  previous comments on splitting that out as well.)

I'm up to nsChromeProtocolHandler.cpp
nsChromeProtocolHandler.cpp:

  Apply same comments as to other version of nsChromeProtocolHandler.cpp!

nsValueArray:

  +        // Implicitly-added entries are not cleared, caller beware
  This comment needs to be in the header, not in the middle of the
  function in the .cpp file!

  While you're there, could you make InsertValueAt return something
  other than false?  And probably remove the |retval| variable too.

  +        if (!GrowArrayBy(aIndex + 1 - Count())) {
  Shouldn't this be aIndex + 1 - Capacity() ?

nsIBinaryInputStream.idl:

  You could make NS_ReadLinkedList a tighter loop and simplify the
  template API by having the classes that are used as template
  parameters implement only a single method, with signature:
    U*& Next()
  and making NS_ReadLinkedList do:
  +    U **prev = aList, *item = nsnull;
  +    for (PRUint32 i = 0; i < count; ++i) {
  +        rv = U::Read(aStream, &item);
  +        NS_ENSURE_SUCCESS(rv, rv);
  +        *prev = item;
  +        prev = &item->Next();
  +    }
  +    *prev = nsnull;
  Not sure if you want to restrict the templatizable classes that way,
  though.  (The current API could be used for doubly
  linked lists; this one would require a second pass over the whole
  list.  Then again, you didn't hit any doubly-linked lists, and it's
  easy to write a second template.)

  (See also my previous comment about NS_ReadAtom and NS_WriteAtom.)
Comment on attachment 201056 [details] [diff] [review]
a couple of small fixes

Marking r- for now; the main thing for which I'm interested in seeing the next version of the patch is the ReadObject/WriteObject vs. separate loading of imported sheets issue.  I'm also having second thoughts about ReadObject/WriteObject for CSS rules (including important) and maybe also mDOMRule.
Attachment #201056 - Flags: review?(dbaron) → review-
(In reply to comment #23)
>   I notice you decided to fill in the sheet pointers in nsCSSStyleSheet
>   by triggering new loads from @import rules.  An alternative, perhaps
>   more consistent with the fastload design, would be to use ReadObject /
>   WriteObject for all of these pointers (generally
>   NS_ReadOptionalObject, etc.).  This seems less risky to me, actually,
>   since there's less worrying about whether you should or should not
>   write out each member -- you just write out everything.  (For example,
>   should you write nsCSSStyleSheetInner::mComplete?).  This would mean

What you're proposing would have the effect, I think, of fastloading non-chrome sheets that are @imported by chrome sheets.  That seems like a bad idea, since we have no way to track dependencies for invalidating these (consider the case where I @import http://www.mozilla.org/foo.css).
Ah, good point.  I suppose we should allow that, although I'm not entirely convinced.
(In reply to comment #27)
> Ah, good point.  I suppose we should allow that, although I'm not entirely
> convinced.
> 

Allow fastloading @imported non-chrome sheets?  That seems like it could break all sorts of things if an extension tried to do this -- the file would never be revalidated from the remote server.  I think having the CSSLoader fallback lends itself to the way the deserialization works in my current patch.

The other option, I guess, would be to not write out a chrome sheet that references any non-chrome sheets (examined recursively).  I don't really see what it buys us over what I did though.  In particular, if we ever wanted to try fastloading CSS for web content, we'd definitely need to be prepared for the possibility that a child sheet needs to be slow-loaded.
(FWIW, consider this a review+ once my comments are addressed, although I still definitely want Boris to look at the sheet loading stuff, since I'm not familiar with that code and he is.)
> * being architecture-independent is valuable

Except for UNIX systems, the XUL fastload file now lives on the local filesystem.  It's a bug that it doesn't live on the local filesystem on UNIX systems (OSX excluded).  So, why is arch-independence valuable?
What is a "local filesystem", exactly, when the home directory is in NFS or AFS?
> What is a "local filesystem", exactly, when the home directory is in NFS or
> AFS?

exactly.  that is why this is a hard problem for the linux desktop.  clearly, the linux desktop needs to define a location on the filesystem that is ideal for network caches.  system services have access to /var/cache for this purpose, and perhaps we could make use of /var/tmp, but that location is typically garbage collected.

as i said before, it is an open bug to properly define the location of the various gecko caches on a linux desktop.  so, we need help from the linux desktop community.
This has most of dbaron's comments addressed, but not all.  In particular, I left child sheet loading using the CSSLoader instead of Read/WriteObject (for the reasons I stated earlier), and I also haven't verified that I'm doing the right thing with the media lists.  I'm not going to have time to work on this in the short term, so I'm posting this in the hopes that someone else can finish it off.
Attachment #201056 - Attachment is obsolete: true
Attachment #201057 - Attachment is obsolete: true
Attachment #201056 - Flags: superreview?(brendan)
Attachment #201056 - Flags: review?(bzbarsky)
Assignee: bryner → dbaron
QA Contact: pawyskoczka → ian
Target Milestone: mozilla1.9alpha → ---
Is it possible to check in what's done and do a new bug for what's left? Or has that already been done? Or does the whole thing need to be together for it to work? Speed improvements, especially startup speed, are always a good thing, and if the sooner the better.
> Is it possible to check in what's done and do a new bug for what's left?

If the correctness issues are addressed, and if there is actually someone to do what's left, yes.

> Speed improvements, especially startup speed, are always a good thing,

If the code is correct.
Blocks: 91242
Assignee: dbaron → nobody
QA Contact: ian → style-system
I try to update the old patch.
it works in Fire Fox Alpha 3 Build 7 ( Gran Paradiso )

Adding CSS Fastload support to your builds simply requires you to add the following line to your .mozconfig file.

ac_add_options --enable-css-fastload
Did you address the remaining review comments (see comment 33) in that patch?

We really don't want the #ifdefs or the configure option; if this patch were complete we'd just want to check it in.

I probably won't have a chance to look until after the holidays.
(In reply to comment #37)
> Did you address the remaining review comments (see comment 33) in that patch?
> 
> We really don't want the #ifdefs or the configure option; if this patch were
> complete we'd just want to check it in.
> 
> I probably won't have a chance to look until after the holidays.
> 
no, I don't complete your comments. 
This patch just modifies some source code for build in granparadiso alpha7.

Keywords: mobile
Whiteboard: startup
In Firefox, we spend a significant amount of time in EnsureUAStylesheet early in startup, way before we get to the bulk of the chrome code.
Blocks: 447581
Flags: blocking1.9.2?
Flags: wanted-fennec1.0+
Whiteboard: startup → startup [ts]
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
renominating -- i think this is pretty important if we're to get our platform startup time down.
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Stuart: could be -- can you share some profile data to help the prioritization of this?  It'd be a late addition to 1.9.2, so we should make sure that we all understand how it prioritizes against other such work.
> Stuart: could be -- can you share some profile data to help the prioritization
> of this?  It'd be a late addition to 1.9.2, so we should make sure that we all
> understand how it prioritizes against other such work.

I'm pretty sure Taras has some profile data with CSS loading/parsing in it.(In reply to comment #41)
> > Stuart: could be -- can you share some profile data to help the prioritization
> > of this?  It'd be a late addition to 1.9.2, so we should make sure that we all
> > understand how it prioritizes against other such work.
> 
> I'm pretty sure Taras has some profile data with CSS loading/parsing in it.(In
> reply to comment #41)

see 'css' attachment part of bug 466739

I think optimizing CSS is one of the few remaining "easy" wins in startup perf on n810
This doesn't sound easy or risk-free at all... given the time/risk constraints we've already talked about for 1.9.2, this seems like something we'd want for 1.9.3, but I don't see it making sense to block 1.9.2.
Blocks: 506431
How much of startup is "parsing CSS"?

And what portion of that time is the "parsing" rather than the creating of data structures, which we'd still have to do with fastload?

Would fastloading CSS make a bigger difference than making some of the data structures less malloc-happy?


If we did this we'd need to leverage our existing CSS tests for property values, selectors, and media queries to make sure they all still function correctly after being serialied and dserialized.  (Just like we have cloning tests for the latter two, although still not for property values.)
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(In reply to comment #45)
> How much of startup is "parsing CSS"?
> 
> And what portion of that time is the "parsing" rather than the creating of data
> structures, which we'd still have to do with fastload?

I don't have a profile for parsing css minus memory allocation. I don't have my oprofile log anymore, but  http://people.mozilla.com/~tglek/fennec/calls.zip will show you sort of callstacks we see during startup. Since this was produced by naively instrumenting the code, timing is a bit wonky for deep callstacks.

> 
> Would fastloading CSS make a bigger difference than making some of the data
> structures less malloc-happy?

I believe so. Looks like we we need to tweak are loading algorithms, not just memory allocation.
(In reply to comment #44)
> This doesn't sound easy or risk-free at all... given the time/risk constraints
> we've already talked about for 1.9.2, this seems like something we'd want for
> 1.9.3, but I don't see it making sense to block 1.9.2.

Where are the flags to ? blocking 1.9.3? Thanks.
I don't think the release-blocking process is the right way to get this bug fixed.
Depends on: 520309
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 18 votes and 57 CCs.
:emilio, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(emilio)

We added some infra for this in bug 1474793, it's unclear whether more is needed. We could save the stylesheets to disk and load them using the ToShmem mechanism I suppose.

Flags: needinfo?(emilio)
See Also: → 1474793
You need to log in before you can comment on or make changes to this bug.