Open
Bug 196843
Opened 21 years ago
Updated 5 months ago
Fastload CSS style sheets
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
NEW
People
(Reporter: janv, Unassigned)
References
(Blocks 2 open bugs)
Details
(Keywords: mobile, perf, Whiteboard: startup [ts])
Attachments
(2 files, 4 obsolete files)
328.46 KB,
patch
|
Details | Diff | Splinter Review | |
111.29 KB,
patch
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•21 years ago
|
Priority: -- → P3
Target Milestone: --- → mozilla1.6alpha
This should wait until after I'm done bug 125246.
Component: XP Apps → Style System
Depends on: 125246
Reporter | ||
Comment 2•21 years ago
|
||
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.
Comment 3•20 years ago
|
||
(In reply to comment #1) > This should wait until after I'm done bug 125246. Looks like that one is done. ;-)
Reporter | ||
Comment 4•20 years ago
|
||
Sorry, I don't plan to work on this in near future.
Comment 5•20 years ago
|
||
In that case, should the milestone be bumped, or the bug just marked as WONTFIX?
Reporter | ||
Updated•20 years ago
|
Target Milestone: mozilla1.6alpha → Future
*** Bug 175410 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Flags: blocking1.8b4?
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4-
Updated•19 years ago
|
Assignee: Jan.Varga → bryner
Target Milestone: Future → mozilla1.9alpha
Comment 7•19 years ago
|
||
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)
Comment 8•19 years ago
|
||
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.
Comment 10•19 years ago
|
||
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 11•19 years ago
|
||
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?
Comment 12•19 years ago
|
||
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?
Comment 13•19 years ago
|
||
Updated•19 years ago
|
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.
Comment 15•19 years ago
|
||
(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.
Comment 20•19 years ago
|
||
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-
Comment 26•19 years ago
|
||
(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.
Comment 28•19 years ago
|
||
(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.)
Comment 30•19 years ago
|
||
> * 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?
Comment 31•19 years ago
|
||
What is a "local filesystem", exactly, when the home directory is in NFS or AFS?
Comment 32•19 years ago
|
||
> 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.
Comment 33•19 years ago
|
||
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)
Updated•18 years ago
|
Assignee: bryner → dbaron
QA Contact: pawyskoczka → ian
Target Milestone: mozilla1.9alpha → ---
Comment 34•18 years ago
|
||
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.
Comment 35•18 years ago
|
||
> 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.
Assignee: dbaron → nobody
QA Contact: ian → style-system
Comment 36•17 years ago
|
||
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.
Comment 38•17 years ago
|
||
(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.
Comment 39•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: blocking1.9.2?
Updated•15 years ago
|
Flags: wanted-fennec1.0+
Updated•15 years ago
|
Whiteboard: startup → startup [ts]
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Comment 40•15 years ago
|
||
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.
Comment 42•15 years ago
|
||
> 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)
Comment 43•15 years ago
|
||
> > 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
Comment 44•15 years ago
|
||
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.
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-
Comment 46•15 years ago
|
||
(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.
Comment 47•15 years ago
|
||
(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.
Updated•2 years ago
|
Severity: normal → S3
Comment 49•1 year ago
|
||
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)
Comment 50•1 year ago
|
||
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.
Description
•