Closed Bug 520309 Opened 15 years ago Closed 14 years ago

startup cache - replacement for fastload cache

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: benedict, Assigned: benedict)

References

(Blocks 6 open bugs)

Details

(Whiteboard: [ts][2010Q1])

Attachments

(4 files, 42 obsolete files)

54.44 KB, patch
Details | Diff | Splinter Review
20.04 KB, patch
Details | Diff | Splinter Review
1.39 KB, patch
dwitte
: review+
Details | Diff | Splinter Review
61.90 KB, patch
Details | Diff | Splinter Review
Rewrite fastload cache to provide convenient API and perf wins. This cache is meant to replace current fastload, manifest, pref caches, etc.

More details in:
https://wiki.mozilla.org/StartupCache and some of the comments in
https://bugzilla.mozilla.org/show_bug.cgi?id=412796
Blocks: 459117
Whiteboard: [ts]
Blocks: 104170
Attached patch pretty much does nothing (obsolete) — Splinter Review
this adds modules/libcache/nsStartupCache.h, which currently has no consumers. Added a line in mozJSComponents that does nsStartupCache::DoTest(), so that I can do testing.

Right now the implementation opens and closes a new nsZipWriter for each write to the cache. We expect callers to serialize all their data into one buffer, then call us once (per module?) and give us an ID to write it. The data can be retrieved by calling .getBuffer(ID), which returns a buffer memory-mapped from the JAR.

Next steps are to convert some caller of nsFastloadService to use nsStartupCache, and also to figure out how this works with our build system (what tier it should go into, etc). Right now it's just all in one .h file which is set as an export in the Makefile.

Feedback is helpful, of course, but I'm not sure the code is in a state to be looked at. I'll post another patch when I have done one or both of the above steps.
Amusingly, the last patch _actaully_ did nothing, because I didn't include the main file.

By the way, the reloadHeaders()/buildFileList() stuff isn't working right now, it's an attempt to get the jar reader a"refreshed" view of an opened JAR after someone does a write to it. (Append writes work fine right now, but not other kinds.) Fortunately, we think that we should only ever have append writes.
Attachment #407081 - Attachment is obsolete: true
For clarification: non-append writes are dealt with right now by just reopening the jar. But for real usage, we don't think that that will happen.
We've been steering new code away from modules for years, perhaps not lately in a loud voice (or any voice). It is a holdover from the old "lib/cmd/include" Unix-y (system V Unix-y, even) Netscape source tree structure.

These days, everything is a module (or almost everything) so there's no value in a "modules" subdir and it costs extra typing.

Is "libcache" stripped of "lib" the right name, i.e. "cache"? We have caches for images, memory and disk page caching, and other uses.

/be
(In reply to comment #4)
> We've been steering new code away from modules for years, perhaps not lately in
> a loud voice (or any voice). It is a holdover from the old "lib/cmd/include"
> Unix-y (system V Unix-y, even) Netscape source tree structure.
> 
> These days, everything is a module (or almost everything) so there's no value
> in a "modules" subdir and it costs extra typing.
> 
> Is "libcache" stripped of "lib" the right name, i.e. "cache"? We have caches
> for images, memory and disk page caching, and other uses.

It should be called startupcache. I suggested that Ben put it into modules/ because that seems like a place that can link other useful parts of Mozilla. Perhaps it should go into /xpcom somewhere, but the dependency on nsZipArchive will make that tricky.
Source location doesn't need to be related to link ordering, we can build mozilla/startup-cache in any tier we need to.
Ben,
I was looking at some oprofile traces and checksumming fastload consistently shows up > 1%. I was also wondering how easy it would be to repack these files, or maybe rely completely on them instead of the plain files that are currently used it.

So that got me thinking, why not try sqlite. It provides a robust way of storing files AND it could lead us towards amalgamating most of the little files we read within user dir into 1 sqlite file.

What do you think of doing a prototype on top of sqlite to see if sqlite can match .mfasl performance of now?
I'd be surprised if sqlite can beat an ad-hoc scheme (even one not tuned since 2001, but we can tune fastload -- although isn't this bug about a much faster new fastload?). ACID is costly. Fast-loading compiled content languages wants only C and D, not A or I. Database is not the hammer for every nail.

/be
(In reply to comment #8)
> I'd be surprised if sqlite can beat an ad-hoc scheme (even one not tuned since
> 2001, but we can tune fastload -- although isn't this bug about a much faster
> new fastload?). ACID is costly. Fast-loading compiled content languages wants
> only C and D, not A or I. Database is not the hammer for every nail.

Note that right now i think a database may beat fastload perf. The fact that we read the whole file in order to checksum it and then only read half of it during normal startup has to hurt.

Furthermore sqlite seems to provide a number of API levels such that one can work on a btree level. Also sqlite has a rep for cutting ACID corners. I'm not yet convinced it will be a performant-enough solution, but I think it's worth investigating.
(In reply to comment #9)
> (In reply to comment #8)
> > I'd be surprised if sqlite can beat an ad-hoc scheme (even one not tuned since
> > 2001, but we can tune fastload -- although isn't this bug about a much faster
> > new fastload?). ACID is costly. Fast-loading compiled content languages wants
> > only C and D, not A or I. Database is not the hammer for every nail.
> 
> Note that right now i think a database may beat fastload perf. The fact that we
> read the whole file in order to checksum it and then only read half of it
> during normal startup has to hurt.

That's a bug to fix, a spot-fix for old fastload. This bug is about a new fastload approach that needs neither database random access with atomic and isolated updates, nor whatever old bug is causing us to write more than we need to read.

Did you file a bug on the write 2x what's read back problem?

> Furthermore sqlite seems to provide a number of API levels such that one can
> work on a btree level. Also sqlite has a rep for cutting ACID corners. I'm not
> yet convinced it will be a performant-enough solution, but I think it's worth
> investigating.

Ok, but I wonder how it'll be validated to make the scheme crashproof. We do not need atomicity or isolation but we do need a single consistency check on startup.

/be
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > I'd be surprised if sqlite can beat an ad-hoc scheme (even one not tuned since
> > > 2001, but we can tune fastload -- although isn't this bug about a much faster
> > > new fastload?). ACID is costly. Fast-loading compiled content languages wants
> > > only C and D, not A or I. Database is not the hammer for every nail.
> > 
> > Note that right now i think a database may beat fastload perf. The fact that we
> > read the whole file in order to checksum it and then only read half of it
> > during normal startup has to hurt.
> 
> That's a bug to fix, a spot-fix for old fastload. 
> Did you file a bug on the write 2x what's read back problem?

Good point. filed bug 523429.


> This bug is about a new
> fastload approach that needs neither database random access with atomic and
> isolated updates, nor whatever old bug is causing us to write more than we need
> to read.

yes and no. I think as we push more stuff into fastload(which we have to) we'll end up needing updates more. I agree that we don't need atomicity, etc.


> > Furthermore sqlite seems to provide a number of API levels such that one can
> > work on a btree level. Also sqlite has a rep for cutting ACID corners. I'm not
> > yet convinced it will be a performant-enough solution, but I think it's worth
> > investigating.
> 
> Ok, but I wonder how it'll be validated to make the scheme crashproof. We do
> not need atomicity or isolation but we do need a single consistency check on
> startup.

Well my thinking is that sqlite is a good persistent storage format and we may benefit from being able to leverage that instead of reinventing another one on top of jar or from scratch.

I think we need to be more agressive about incremental updates. For example instead of wiping out fasl files during component reg or version updates, we should be able to selectively update js components that need updating. Ie 99% of the time there will be no need to rewrite the fasl file from scratch AND load js components the slow way.
One benefit if reusing jar structure is that we *could* move stuff out from system jars into the startup cache AND order everything in sequential fashion.
Summary: replacement for fastload cache → startup cache - replacement for fastload cache
Replaced instances of fastload with startupcache in mozJSComponentLoader and XULPrototypeCache. startupcache keeps writes in memory until explicitly told to write to disk.

Right now, startupCache also keeps a map of open input and output streams, to match fastload's support for partial reads and writes. I am considering factoring this behavior out of the startupCache.

StartupCache is also currently just one .h file in modules/libjar, and nsIStartupCache.idl lives in xpcom/io for build reasons. I wasted a couple of days on build stuff early on, and I figured I'd just resolve it all at once after getting some feedback. I'd suggestions on where the cache should end up living, and of course I'll be splitting it into .h/.cpp as well.
Attachment #407086 - Attachment is obsolete: true
From the department of questionable statistics:

with patch, cold
12857 12855 11903 12662 11328
av: 12321

without patch, cold
12651 12281 13970 12336 12012
av: 12 650
Depends on: 529170
Blocks: 506392
Depends on: 529173
Blocks: 529170, 529173
No longer depends on: 529170, 529173
+    sc->HasData(urispec.get(), &exists);
Get rid of HasData. sc should just fail when one goes to request data that isnt there, something like GetInputStream

+                NS_WARNING("inline script in nsXULElement, not sure about how to"
+                           "handle this but should be ok?");
Ask shaver about this one?

You are right that JARInputStream is the wrong interface for this. Please factor streams out of the sc.
Startup cache io would all go either directly via nsStartupCache::GetBuffer or some inmemory stream constructed outside of startup cache.

Also, we shouldn't be adding this as a oldschool IDL xpcom service. Should add sc as a concrete class without any virtual functions.

We need to find a place for this to live. I think it should move under /xpcom somewhere.

As I mentioned on irc, startup cache core needs to be a separated from the rest of the changes.

Generally, the code looks good. Most of the changes to zipreader seem reasonable.
Summary of IRC conversation about future work:

Taras recommends that I simplify API to getBuffer/putBuffer only (accepts only memory buffer, not streams), and supply some utility methods to wrap these in streams when necessary.

This is a little tricky, especially for putBuffer. Current consumers use objectOutputStream for write, so to call putBuffer they need to do: create storageStream, wrap with objectOutputStream, do write, get inputStream from storageStream, read from inputStream to buffer, call putBuffer. Also, clients like xulPrototypeCache would then need to handle their own map of read/write streams, which used to be handled by fastload.

Also from IRC, was recommended to put this code in [top level]/startupcache.

One obstacle to de-XPCOM-ifying this is that startupCache needs nsZipWriter/nsZipArchive, but cache's consumers get built before libjar does. We weren't sure if bug 528250 will help with this.
So the refactoring work is done. One snag that I ran into while moving my code to /startupcache is that I use nsZipArchive in my file, but there's no XPCOM interface for it. Seems like my solutions are 1) use a different interface, like nsJARStreams, 2) make an XPCOM interface for nsZipArchive, or 3) inline a bunch of functions in nsZipArchive.

Are there any other options? We feel that streams aren't the right solution, and Taras is opposed to XPCOM-ifying nsZipArchive.
Attachment #412355 - Attachment is obsolete: true
Ben, is this work ready for review?
Yesterday I found a mysterious test failure caused by the core patch that I am looking into now. 

>WARNING: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != 
>BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file 
>/Users/bhsieh/clean-moz/xpcom/base/nsTraceRefcntImpl.cpp, line 956

I thought I had fixed this before, but I am looking into it again.
Attached patch only startup cache (obsolete) — Splinter Review
includes a few makefile fixes that didn't make it into the last patch. test failures seem to have disappeared...
Attachment #414787 - Attachment is obsolete: true
Attachment #415686 - Flags: review?
Now we checksum the entry when getBuffer() is called, we remove the whole cache file if we detect corruption.

Partially to support this, getBuffer() returns a malloc'd buffer instead of a mmaped buffer. The startupCacheUtils code handles deallocation if clients use it to wrap the returned buffers into streams, otherwise callers take ownership of the buffer.
Attachment #415686 - Attachment is obsolete: true
Attachment #416210 - Flags: review?
Attachment #415686 - Flags: review?
Attachment #416210 - Flags: review? → review?(benjamin)
department of questionable statistics report:

with patch (both patches), cold
8921 9768 7454 8898 9735 9154 9199
average is 9018

without patch, cold
10814 8607 11084 10742 8863 10852 8795
average is: 9965

I'm slightly more skeptical of these numbers than usual, my machine has been a little weird today. Can re-run numbers if that would be helpful.
Should have said: above numbers were with opt build, preliminary numbers in comment 14 were with non-opt build. Could account for some discrepancies.
Blocks: 94199
Blocks: 196843
I probably won't get to this until next week, 15-Dec
Attachment #416210 - Flags: review?(benjamin) → review?
Comment on attachment 416210 [details] [diff] [review]
startupCache only, does checksumming and memcpy in getBuffer

While going over this patch, found one bug in certain builds and another more substantial bug. Hold off on review, hoping to have another patch up soon.
Attachment #416210 - Flags: review?
Priority: -- → P1
Blocks: 512799
Basic summary of the problem I'm thinking about:

ObjectOutputStream (and BinaryStream, which I'm using) support three methods: WriteObject, WriteSingleRefObject, and WriteCompoundObject. 

I kind of naively and fuzzily assumed that Read/WriteObject would take care of the case with multiple strong pointers to the same object (otherwise we'd only implement WriteSingleRefObject, right?)

Upon closer inspection, nsBinaryStream's ReadObject just spawns new objects each time it's called; in other words, it does not handle the case with multiple strong pointers. One might say that it should just throw NS_ERROR_NOT_IMPLEMENTED and only implement ReadSingleRefObject, but that's not the case.

We felt that we wanted to keep the core API of the startupCache very simple (ie, no streams, only buffers). So it seems that the client who need this functionality will need to take care of guaranteeing that each object is only written once and appropriately resolving the pointers and so on.

Probably I'll modify or subclass BinaryStreams so that you can get/set an map of objects and their information, and clients will need to care take of maintaining and serializing this map.
Group: mozilla-confidential
Group: mozilla-confidential
(In reply to comment #28)
> Basic summary of the problem I'm thinking about:
> 
> ObjectOutputStream (and BinaryStream, which I'm using) support three methods:
> WriteObject, WriteSingleRefObject, and WriteCompoundObject. 

nsBinaryStream.cpp concrete classes inherit from both the nsIBinary*Stream and nsIObject*Stream interfaces, with stub impls for the latter, to avoid yet MI overhead:

NS_IMPL_ISUPPORTS3(nsBinaryOutputStream, nsIObjectOutputStream, nsIBinaryOutputStream, nsIOutputStream)
...
NS_IMPL_ISUPPORTS3(nsBinaryInputStream, nsIObjectInputStream, nsIBinaryInputStream, nsIInputStream)

> I kind of naively and fuzzily assumed that Read/WriteObject would take care of
> the case with multiple strong pointers to the same object (otherwise we'd only
> implement WriteSingleRefObject, right?)

Did you read the doc comments in xpcom/io/nsIObjectOutputStream.idl?

I split out WriteSingleRefObject to allow callers to optimize significantly by avoiding hashing the root isupports pointer, etc. We cannot assume the refcount (which is readable only with an AddRef()/Release() pair, so also expensive) tells us that the object was single-ref *within the graph being serialized*. It could be 2 (one for caller, one for that graph) but it could also be more.

The rest of the API follows, as the doc comments 'splain. This is all pre-CC and I'm not sure we can use CC classinfo extensions to avoid it.

But you cannot simply shunt it off onto multiple clients of the startup cache, or they will all reinvent the "sharp/dull" identity-hashtable wheel. Why not provide it as part of the lowest layer service-level API?

/be
Brendan, originally Taras and I hoped to avoid putting this sort of complexity into the startupcache to reinforce the idea that it's just meant for "flat blobs". I think you're right that at this point it's better just to bite the bullet and just provide it, though. At least we get a unified cache out of it.

Re: doc comments, in ObjectOutputStream.writeObject comments say 
>If the object has only one strong reference in the serialization and no weak 
>refs, use writeSingleRefObject."

But if the object has multiple strong references in the serialization, concrete implementing classes in nsBinaryStream.cpp do the wrong thing (ie, result in multiple copies), right? I'm not entirely sure what you mean by a stub impl in this case.

Sounds like what you are saying is this: consumers of concrete nsBinaryStream call WriteObject if there is one strong reference in the serialization, perhaps with other references in memory. They call WriteSingleRefObject if there is one strong ref in the serialization, with no other references in memory. If there are other strong refs in the serialization, nsBinaryStream may not be used.
(In reply to comment #30)
> Re: doc comments, in ObjectOutputStream.writeObject comments say 
> >If the object has only one strong reference in the serialization and no weak 
> >refs, use writeSingleRefObject."
> 
> But if the object has multiple strong references in the serialization, concrete
> implementing classes in nsBinaryStream.cpp do the wrong thing (ie, result in
> multiple copies), right?

Not as I originally designed and implemented:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/io/nsBinaryStream.cpp&rev=1.9

Looks like bz changed things (I sr'ed and subsequently forgot ;-) for bug 390474:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/io/nsBinaryStream.cpp&rev=1.29

But this does not fulfill the contract, obviously. My NS_ERROR_NOT_IMPLEMENTED does (failure is always an option with XPCOM :-P). I should have raised this issue when sr'ing the patch for bug 390474.

> I'm not entirely sure what you mean by a stub impl in
> this case.

See rev1.9 in cvs.m.o.

> Sounds like what you are saying is this: consumers of concrete nsBinaryStream
> call WriteObject if there is one strong reference in the serialization, perhaps
> with other references in memory. They call WriteSingleRefObject if there is one
> strong ref in the serialization, with no other references in memory. If there
> are other strong refs in the serialization, nsBinaryStream may not be used.

This is a fair description of how things evolved, but in my original design and implementation, the *only* place non-error'ing impls were provided was over in xpcom/io/nsFastLoadFile.cpp.

/be
Now handles multiply-referenced objects.

This still needs tests. The main problem I am running into with those is, I want to create a few dummy class to work out all the aspects of serialization. Then, I'd need to register it with XPCOM in my compiled-code test, so that I can instantiate it with do_CreateInstance() in my test. (Both fastload and startupcache write out the object CIDs and call do_CreateInstance during deserialization. So far as I can tell, Fastload deals with this by not having tests ;)
Attachment #416210 - Attachment is obsolete: true
mozJSComponentLoader and nsXULPrototypeCache updated to use startupCache's streams; this makes their serialization work correctly for multiply-referenced objects.
Attachment #414788 - Attachment is obsolete: true
Next steps on this are:
- Optimize out some memcopies, better standardization of which functions take ownership of pointers you pass in. (Right now, all of them do, except that they don't take ownership of the string identifier you pass in.)
- Do some timing.
- Write some comments, in particular explaining the layout of the different structures I use to keep track of object data and object references, how they look in memory and how they look on disk.
- Write tests, as explained above. bsmedberg gave me some pointers on how to do that kind of registration, but he thinks it will be tricky.

I would definitely appreciate suggestions on how to test serialization without making my own dummy classes. Maybe there are some simple, easy-to-instantiate classes that also implement nsISerializable? I'd also take feedback on the code (although possibly you want to wait for better comments before wading in).
So far, tests are written, timing is fluctuating (but seems to be a regression overall). Next step would be to support updating a cache file in some reasonable way (turns out that this actually happens all the time, which we didn't realize initially).

I discovered while looking into this that the patch I posted a month ago: https://bugzilla.mozilla.org/attachment.cgi?id=416210 actually does work correctly. The reason is that (so far as I can tell) the only object which is serialized multiple times by current clients is the nsSystemPrincipal, which is a singleton with no serialized data. Thus, simply writing the nsID is actually sufficient to ensure correct serialization/deserialization. All other objects (so far as I could tell) are only written once during serialization, so simple nsBinaryStream is capable of handling them as well.

Note that nsBinaryStream also does not handle weak references (this is a subset of multiply-referenced object, although it at least asserts instead of doing something incorrect). No weak references are ever written by current clients, though.

I wonder if we might consider just using that (very naive) implementation? It has a much, much simpler API than fastload, gives us a unified cache, and also produces a small immediate performance win. The disadvantage is of course that we would be unable to serialize multiply-referenced non-singleton objects, although with some changes we would at least be able to detect and assert when this happened.

One other consideration is that current fastload (so far as I can tell) is only able to correctly serialize multiply-referenced non-singleton objects if all of those references are written to the same writer. So if you write such an object, then close the writer, then write it again via fastloadFileUpdater, you will (incorrectly) get two distinct objects out when you deserialize.

So the very naive implementation differs from current fastload only in that a multiply-referenced non-singleton object written multiple times before writer close will not be serialized correctly, while current fastload will handle that correctly. Updating should already work correctly in the naive implementation (again assuming all multiply-referenced objects are singletons).
(In reply to comment #35)
> So far, tests are written, timing is fluctuating (but seems to be a regression
> overall). Next step would be to support updating a cache file in some
> reasonable way (turns out that this actually happens all the time, which we
> didn't realize initially).

The nsFastLoadFileUpdater code is a big deal, as source and at runtime. This was a topic in some bugs I was cc'ed on by Taras. Sorry I didn't have time to review here.

> I discovered while looking into this that the patch I posted a month ago:
> https://bugzilla.mozilla.org/attachment.cgi?id=416210 actually does work
> correctly. The reason is that (so far as I can tell) the only object which is
> serialized multiple times by current clients is the nsSystemPrincipal, which is
> a singleton with no serialized data. Thus, simply writing the nsID is actually
> sufficient to ensure correct serialization/deserialization.

Yes.

> All other objects
> (so far as I could tell) are only written once during serialization, so simple
> nsBinaryStream is capable of handling them as well.

Since bz's change the other year, yes.

> Note that nsBinaryStream also does not handle weak references (this is a subset
> of multiply-referenced object, although it at least asserts instead of doing
> something incorrect). No weak references are ever written by current clients,
> though.

FastLoad was from the XPCOM heyday, so it was triple-threat (weak, "sharp" multiple strong refs, and the important "dull" single-ref optimization). We can try to simplify with hindsight, for sure.

/be
> All other objects (so far as I could tell) are only written once during
> serialization

With arbitrary extensions installed?
Good question bz, I will try installing some extensions and try this out. Do you have any suggestions for extensions that might behave differently?
For example, extensions that load XUL documents from chrome://skin/ URIs.  Not sure whether any do...
Attachment #420176 - Attachment is obsolete: true
Attachment #420178 - Attachment is obsolete: true
Addressing bz's question: it's true, a xul document loaded through chrome://<pkgname>/skin will not use nsSystemPrincipal. I think this is pretty unlikely to cause a problem, though (from my limited understanding of the principals). From what I understand, each document will have its own principal (so no multiply-referenced principal) and additionally the URIs in the principal object are immutable (so even if there were multiple references in the serialization, leading to duplicated objects after deserializing, the principals would still compare correctly).

As a safety mechanism, I added a debugObjectOutputStream in debug builds, which keeps a set of objects written and looks writes of multiply-referenced non-singleton objects and asserts if it finds one. With a number of extensions installed (including a hand-rolled one that loads xul through chrome://<...>/skin), I did not hit this case.
Okay, now the terrible, very bad, no good news. 
Timing statistics:

patched:
2257 2302 2216 2206 2309 2397 2148 2377 2333
average: 2282

unpatched:
2162 1875 1985 2577 2498 1858 1893 1785 1841
average: 2052
------

So, a sizable regression. No real idea what to do about this. Reading through old comments, Taras suggested some improvements to my (modified) nsXULPrototypeCache that might lead to some perf gains, I guess I will try that next. By the way, these numbers are generated using the new unmount/mount way of measuring, so that is why they look so different from the ones above, which were obtained with purge/sync. 

Non-debug code should be very, very similar to the code I posted back in December, which seemed to show a small perf gain (albeit with the purge/sync method). So I'm not sure why it seems to be slower now. I did some timing with the purge/sync method on this patch just out of curiosity, and the patch still seems to be a sizable regression so I really don't know what accounts for that. Possibly other changes to the codebase in the meantime?
By the way, I did some timing work on the complex code that handled multiply-referenced objects (the last patch). It showed a similar regression, so I guess fastload is just pretty hard to beat with this approach. These are old numbers, from a few weeks ago. I didn't post this before because the patch was still broken in significant ways.
-----

patched (supporting sharp-object serialization, roughly what I posted in https://bug520309.bugzilla.mozilla.org/attachment.cgi?id=420176):
4070 4073 4045 4080 4003 4138 4039
4064

unpatched:
3719 3798 3799 3769 3834 3813 4209 3792
3841 
---

I don't know why the unpatched numbers moved so much between the times I did the two timing tests. Could be some combination of other codebase changes and different background processes on my laptop? These numbers were done with the mount/unmount method.
> each document will have its own principal (so no multiply-referenced principal)

That may not necessarily be the case in all situations, actually.  Documents can inherit principals from one another.

> so even if there were multiple references in the serialization, leading to
> duplicated objects after deserializing, the principals would still compare
> correctly

If the URIs are chrome, yes.  Can we get into this code for non-chrome-uri principals?
Attachment #424151 - Flags: review?(benjamin)
I figured out why I was showing a regression in the timing, it is because I was doing it wrong. New numbers, 50-run sample of cold startup:

Patched:
3309

Unpatched:
3680

Sweet! bz, I will look into both questions next week. Seems like it should be a mistake to commit anything to fastload that is not from a chrome URI though, right? Also, are you saying that the case of inherited principals would not be a problem if the URIs are chrome:// (specifically chrome://.../skin)?

I asked bsmedberg for a review of the core components, but I will try to have better answers for these questions when we find reviewers for the clients patch. My guess is that, with a carefully sculpted extension, it might be possible to (incorrectly) get duplicate principals, and then it would be maybe possible but even more difficult to make it matter that they were duplicated. I did try installing a number of complex extensions, and some themes, and didn't see anything that even caused the duplicate principals, though.

Again, any of these things also would cause an assertion in debug builds.
> Seems like it should be a mistake to commit anything to fastload that is not
> from a chrome URI

Sure.  We used to do that, though; not sure whether we still do.

> Also, are you saying that the case of inherited principals would not be
> a problem if the URIs are chrome:// (specifically chrome://.../skin)?

I'm saying that specifically principal equality comparisons (which is what I was quoting in comment 45) in that case do not depend on object pointer identity comparisons.

There are other protocols, however, for which they in fact do depend on pointer identity of the nsIURI.

> Again, any of these things also would cause an assertion in debug builds.

Sure, but to a first approximation no one runs debug builds with extensions.  We do run them on tbox on our tests, but assertions are not fatal there...

To be clear, I think the approach being taken is fine; we just need to guard the entrypoints a bit to make sure that we don't end up in this code in situations which might fail. (e.g. should we perhaps not fastload XUL documents whose principal is not system?  Or double-check that only chrome:// URI documents are fastloaded?  What other entrypoints matter?)
Perhaps I'm not reading the bug correctly, but why is this in my review queue? Isn't it a known performance regression? Are you hoping to take it anyway, or use it only in some situations where it's an improvement?
bsmedberg, see comment #46 for latest cold start timing numbers -- seems to be a performance win. I got a regression earlier because I was doing something wrong in the timing tests.

I was hoping to get a review for the core code before continuing to address the issues in client code that bz was talking about above.
Whiteboard: [ts] → [ts][2010Q1]
Blocks: 447581
I'm going to do API review and dwitte will do followup detailed code review.

Overall, I'm concerned that we're using XPCOM needlessly. Are there/will there be any scriptable clients of the startup cache?

none of init(), writeToDisk() or removeCacheFile() should not be part of the public API. The implementation should cooperate with XPCOM startup/shutdown to write the cache to disk automatically, and external clients should have no knowledge of that. If necessary there should be a client-facing API to invalidate the cache, but "removeCacheFile" is at least the wrong *name* for that API.

From reading the startup-cache API I don't understand how it is meant to interact with object output streams. Does the object-outpustream code exist as a porting tool for existing clients of fastload, but it will eventually be replaced? Are there still cases where we're serializing arbitrary object graphs to the startup cache? (Other than XDR data, which has its own object-graph handling). Is their documentation on how that is supposed to work?

The getBuffer API hands back a buffer but doesn't tell the caller its size. At least in debug code you'd typically want to check that the buffer has the size you expect. I think that the best way to deal with this is just use nsCString as the data holder: nsCString can have embedded nulls and are a good container for arbitrary buffer data that may not actually be text data.

Are there restrictions on the "id" string passed to get/setBuffer?

At what times during the bootstrap/runtime/shutdown may clients write to the startup cache? What happens if a client attempts to write an entry that already exists? Does it silently replace or assert? Asserting sounds like the correct behavior to me, since writing the same entry twice implies that you should have checked whether it was fastloaded in the first place, but it may be a problem if you are loading the same resource asynchronously and are in the process of fastloading it.

The documentation should make clear that the cache is not intended to be durable and clients should not save precious data to the cache. The docs should also make clear whether, if you've called putBuffer("someid"), you can reliably getBuffer("someid") in the same session (probably not, since other clients may invalidate the cache).

It's also important to make sure that we don't typically invalidate the startup cache. In the past we've had bugs where if the user had certain preferences set (intl.locale.matchOS or general.useragent.locale) we would throw out the XUL cache on every startup, which is obviously bad behavior.
Comment on attachment 424151 [details] [diff] [review]
startupCache only, no sharp object serialization

sr- based on those comments: catch me on IRC to discuss as appropriate
Attachment #424151 - Flags: review?(benjamin) → superreview-
Comment on attachment 424152 [details] [diff] [review]
clients of startupcache, back to naive no-streams model

DeCOMtaminate! DeCOMtaminate! (in Dalek voice ;-)

Srsly, getBuffer and putBuffer seem to be from something I did (nsIStreamBufferAccess) but getBuffer now allocates. The idea, never realized, with nsIStreamBufferAccess was to get non-copying access to the stream's buffer and XDR out of that memory. New'ing and copying defeats any win in doing that "inlined access".

I suggest dropping getBuffer, or at least renaming it. Does it have a use-case now?

/be
> Srsly, getBuffer and putBuffer seem to be from something I did

... a decade ago.

Not saying inlined buffer access is bad, just that it never happened and it still seems a premature optimization.

If getBuffer survives (name change or not) it should return a char* and avoid an out param, I think. Or nsCString as bsmedberg said, but in that case it's totally unclear what the use-case is.

/be
bsmedberg, thanks for the review! I will catch you in IRC to discuss, I just wanted to clear up some things in the bug.

I decided to use XPCOM in startupcache to try to overcome some build difficulties: the startupcache needs code from libjar, but it's referenced by code like nsXULPrototypeCache, and mozJSComponentLoader, which are built in earlier build tiers.

More important:
getBuffer/putBuffer aren't auxiliary functions of the cache, they are fundamentally what the startupCache is about. The original goal was to have an incredibly simple API with only these two functions. Then, clients can use utility functions supplied in nsStartupCacheUtils to wrap these buffers in streams to get nsIObjectStream functionality, if desired. Other clients, like manifest or pref caches, might not need these streams at all. It's actually pretty unfortunate that we picked the complex clients to switch over to this first, because the cache was designed with simple clients in mind. To answer bsmedberg's question, these streams do allow us to serialize object graphs of a certain complexity and some clients do need that. 

The new/copy on getBuffer is unfortunate, yeah. The alternative is to give back mmapped buffers directly from the jar file. The problem is that these buffers would fail without warning (point to arbitrary data?) if someone mucked with the jar file, or more probably if we needed to remove the cache file after handing out the pointers to mmap'd data. On the bright side, there are no additional copies made for the wrapping streams (if they are used), they just share the buffer.
Note that most build tiers went away in bug 528250.
Attached patch startup cache only (obsolete) — Splinter Review
Attachment #424151 - Attachment is obsolete: true
will provide some notes about these patches tomorrow.
Attachment #424152 - Attachment is obsolete: true
Attached patch startup cache only (obsolete) — Splinter Review
Fixed some documentation from the last patch.
Attachment #442001 - Attachment is obsolete: true
Changes from last time:
Mostly removed XPCOM interface. I created an XPCOM wrapper so tests can access the code, but all other clients can and should access the cache directly as a singleton using static method StartupCache::GetSingleton(). StartupCache is now only available in libxul builds, because of the build stuff needed to make this happen.

Simplified the API to three main calls: GetBuffer, PutBuffer, and InvalidateCache. There is also a GetDebugObjectOutputStream method for debug code, which wraps a provided objectoutputstream and makes sure that no non-singleton object is written more than once. Id-strings given to Get/PutBuffer should be null-terminated character arrays. GetBuffer/PutBuffer do return/require the size of the buffer. 

The StartupCache is available after XPCOM initialization. PutBuffer calls during startup (specifically, before the 'final-ui-startup' call is received) are stored in-memory and written out together on a timeout which fires a few seconds later. PutBuffer calls after this point are written out immediately. PutBuffer calls receieved after XPCOM shutdown starts will return an error. PutBuffer now asserts if a new entry would replace an old one.

I've put some documentation in StartupCache.h about how to use the cache, what it's for, when it's available, etc.

I decided to stick with char* and int lengths instead of using nsCString, because we often want to adopt the buffer with a StringInputStream. With raw char*, we can just forget about the buffer, and the StringInputStream will own it. With nsCString, we'd have to share the data instead of adopting it, and we'd have to keep the nsCString alive along with the StringInputStream.

The test currently only tests post-startup writes. I had some trouble testing startup writes, because of the timer that I'm using to fire the aggregate startup write. I think that the event basically fires after the 'current' stack finishes resolving, so it will fire after the test finishes. We're planning to kick actual I/O onto a separate thread in a follow-up patch, so I'll be able to test it then just by issuing the notification, then PR_Sleeping for a few seconds. (An option to test startup writes before that happens is to run another thread that notifies 'final-ui-startup' and waits for the timeout, but I'm not sure how to tell the thread to wait for the timeout, and it seemed more reasonable to wait for the follow-up bug.)

Client code was updated to work with the new startupcache code. Clients are a bit ugly right now, with a bunch of #ifdefs to make non-libxul builds work (ifdef'ing out references to startupCache). I need to clean them up a little and think about sane things to do in non-libxul builds. I also noticed just now that that hg mangled the client code in the diff, basically deleting and adding the contents of whole files, so the patch isn't really 160KB -- that's just some mangling. Clients do work though, both in libxul and nonlibxul builds (although some basically do nothing in nonlibxul). I'll do some updated timing of these soon, but I don't expect the numbers to move much.
Just for clarification: most of the changes were build configuration or API changes, not implementation changes. PutBuffer implementation changed, in that writes before final-ui-startup notification are written out together and writes afterwards are written immediately. Previously, all PutBuffer buffers would get written to disk on shutdown. The StartupCache is initialized and accessed directly, instead of through do_GetService(), so that's a small implementation change as well.

I don't expect this to impact timing, because all of the startup PutBuffer calls should only happen during one initial run (specifically, whenever buildid changes or cache is otherwise invalidated), and we don't time that. Warm and cold start timing  assume that startup writes were already done in a previous run, and so the timing really only includes GetBuffer calls, the implementation of which didn't change at all.
Attachment #442136 - Flags: superreview?(benjamin)
Attachment #442136 - Flags: review?(dwitte)
Attached patch client mozJSComponentLoader (obsolete) — Splinter Review
Fixed the diff (somehow the line endings got messed up). Also, I separated out the two clients into different patches, because they will probably have different reviewers.
Attachment #442002 - Attachment is obsolete: true
Attached patch client scXULCache (obsolete) — Splinter Review
The other client, nsXULPrototypeCache

The libxul #ifdefs I added in nsXULElement are a little nasty. In client code, I've preferred to just #ifdef out whole methods (no declaration, no definition) if losing access to startupCache totally guts their functionality. But in nsXULElement, the serialize/deserialize methods are virtual methods inherited from nsXULPrototypeNode, and it seemed bad to modify the whole inheritance chain... so in this case I #ifdef'd out the bare minimum necessary to make the thing compile and run. I suspect that this leaves some dead code in non-libxul builds.

When I get a reviewer for this code, would appreciate feedback on what to do there. Again, the #ifdefs are because StartupCache will only be enabled for libxul builds.
Comment on attachment 442136 [details] [diff] [review]
startup cache only

Drive-by: the style guide says to put the left brace of method and function bodies (except for methods defined inline in classes) on its own line in column 1.

/be
Comment on attachment 442136 [details] [diff] [review]
startup cache only

Notes from reading the headers:
* StartupCacheListener shouldn't need to support weak references.
* cacheEntry::cacheEntry has a comment "Takes possession of buf", but it doesn't
* I'm pretty sure StartupCache::mTable should be a nsDataHashtable, not nsClassHash.
* final-ui-startup is only fired for XR apps which go through XRE_main. Since you're also using a timer, we should consider either just starting a timer from startup, or firing off a timer the first time an event loop is pumped.
* The header list on StartupCache.h is way too long, I'm pretty sure you don't need all for the header, just for the impl.
* Where does the startup cache get destroyed? I see ::gShutdownInitiated, but no actual destruction.

It's still not clear to me from the header what happens during our initial startup sequence, i.e. First startup, we find what extensions are installed and immediately restart. Second startup we actually register the XPCOM components in those extensions. Would a startup cache be written at all during the first run, or only the second run?
Attached patch API addition (obsolete) — Splinter Review
I added a method to the API, PutInputStream, that adds the contents of an input stream to the cache. This is to avoid the memcopies in the original process of: make new buffer, read into buffer, adopt buffer into string stream, pass stream to zipwriter. I did not add a corresponding GetInputStream, because I don't want to give the impression that we would manage partially-read streams for the client, like fastload service did.

I modified clients to use these, so I will post those patches as well.

>* StartupCacheListener shouldn't need to support weak references.
Removed.
>* cacheEntry::cacheEntry has a comment "Takes possession of buf", but it
>doesn't
>* I'm pretty sure StartupCache::mTable should be a nsDataHashtable, not
>nsClassHash.
The in-memory write cache now stores input streams, instead of buffers. They have to be streams to be passed to the ZipWriter, anyways.
>* final-ui-startup is only fired for XR apps which go through XRE_main. Since
>you're also using a timer, we should consider either just starting a timer from
>startup, or firing off a timer the first time an event loop is pumped.
Now firing off a 10-second timer in Init(), which is called just before xpcom-startup. (Directly called by code in nsXPComInit.)
>* The header list on StartupCache.h is way too long, I'm pretty sure you don't
>need all for the header, just for the impl.
Removed some of these.
>* Where does the startup cache get destroyed? I see ::gShutdownInitiated, but
>no actual destruction.
I actually call the destructor directly in NS_ShutdownXPCOM(). I thought that is what we discussed, but maybe I misunderstood.
Attachment #442136 - Attachment is obsolete: true
Attachment #444055 - Flags: superreview?(benjamin)
Attachment #442136 - Flags: superreview?(benjamin)
Attachment #442136 - Flags: review?(dwitte)
Attachment #442521 - Attachment is obsolete: true
Attachment #442524 - Attachment is obsolete: true
I didn't mention before -- I actually fixed the test, so now it tests both startup writes (which go to an in-memory cache first and get written out to disk all together) and post-startup writes (which go directly to disk).
I really don't like the stream stuff. Why do we have to use streams and nsIZipWriter, instead of buffers and nsZipArchive? Streams are going to make it difficult to do the writes off the main thread, which is a near-term goal after this lands I hope!
Attached patch revert API change (obsolete) — Splinter Review
Removes PutInputStream() over the following concerns: Hard to mark inputstreams as 'final' or 'immutable', and also what happens if client calls GetEntry on an entry that is in-memory in stream form? Also, fixes bracket style that be pointed out.

The two clients will be reverted also.

>* I'm pretty sure StartupCache::mTable should be a nsDataHashtable, not
>nsClassHash.
I think if you accept that we need raw buffer / length pairs for the reasons I said in comment 59, we want nsClassHash here. nsDataHash assumes simple types, so it does a lot of copy construction, destruction on method exit, etc. This is bad for my struct.
Attachment #444055 - Attachment is obsolete: true
Attachment #444055 - Flags: superreview?(benjamin)
Attachment #444056 - Attachment is obsolete: true
Attachment #444057 - Attachment is obsolete: true
Attachment #442521 - Attachment is obsolete: false
Attachment #442524 - Attachment is obsolete: false
Attachment #444218 - Flags: superreview?(benjamin)
Attached patch startup cache only, better test (obsolete) — Splinter Review
Attachment #444218 - Attachment is obsolete: true
Attachment #444790 - Flags: review?(dwitte)
Attachment #444218 - Flags: superreview?(benjamin)
Attachment #442521 - Flags: review?(brendan)
Attachment #442524 - Flags: review?(jonas)
Comment on attachment 444790 [details] [diff] [review]
startup cache only, better test

>diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp

>@@ -688,16 +688,31 @@
> 
>   // -- check if there is enough source data in the file
>   if (offset + aItem->Size() > len)
>     return nsnull;
> 
>   return data + offset;
> }
> 
>+PRBool 
>+nsZipArchive::CheckCRC(nsZipItem* aItem, const PRUint8* itemData) {
>+  if (!aItem)
>+    return PR_FALSE;
>+
>+  if (!itemData) {
>+    itemData = GetData(aItem);
>+    if (!itemData)
>+      return PR_FALSE;
>+  }

See comments at callsites below, but I think you can ditch the first nullcheck and the !itemData branch, and also ditch the 'itemData = NULL' default. If these invariants should be true, let's enforce them by crashing. ;)

>+  PRUint32 crc = crc32(0L, (const unsigned char*)itemData, aItem->Size());

Nit: s/0L/0/ will work just fine.

>@@ -707,21 +722,18 @@
>     return NS_ERROR_FILE_CORRUPTED;
> 
>   if (outFD && PR_Write(outFD, itemData, item->Size()) < (READTYPE)item->Size())
>   {
>     //-- Couldn't write all the data (disk full?)
>     return NS_ERROR_FILE_DISK_FULL;
>   }
> 
>-  //-- Calculate crc
>-  PRUint32 crc = crc32(0L, (const unsigned char*)itemData, item->Size());
>-  //-- verify crc32
>-  if (crc != item->CRC32())
>-      return NS_ERROR_FILE_CORRUPTED;
>+  if (!CheckCRC(item, itemData))
>+    return NS_ERROR_FILE_CORRUPTED;

We're guaranteed item && itemData here.

>diff --git a/startupcache/Makefile.in b/startupcache/Makefile.in

>@@ -0,0 +1,82 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2009
>+# the Initial Developer. All Rights Reserved.

Netscape? Really? ;)

You should use the licenses at http://www.mozilla.org/MPL/boilerplate-1.1/ for all new files.

>+DEPTH		= ..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@

Use spaces (tabs are misaligned)

>+MODULE = startupcache
>+MODULE_NAME = StartupCacheModule
>+LIBRARY_NAME = startupcache
>+SHORT_LIBNAME = scache
>+EXPORT_LIBRARY = 1
>+LIBXUL_LIBRARY = 1
>+IS_COMPONENT = 1
>+GRE_MODULE = 1
>+#FORCE_STATIC_LIB = 1
>+#MOZILLA_INTERNAL_API = 1

Remove the # lines.

>+XPIDLSRCS = nsIStartupCache.idl \
>+  $(NULL)

Could you #ifdef ENABLE_TESTS this, and the test cpp's? (See later)

>diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp

>+nsresult
>+StartupCache::InitSingleton() 
>+{
>+  nsresult rv;
>+
>+  NS_ASSERTION(StartupCache::gStartupCache == nsnull,
>+               "InitSingleton should only be called once!");
>+
>+  StartupCache::gStartupCache = new StartupCache();
>+  if (!StartupCache::gStartupCache)
>+    return NS_ERROR_OUT_OF_MEMORY;

No need for OOM checks on small amounts of data.

>+StartupCache::~StartupCache() 
>+{
>+  WriteToDisk();
>+  mTable.Clear();
>+#ifdef DEBUG
>+  mWriteObjectMap.Clear();
>+#endif

mTable and mWriteObjectMap will autoclear themselves on destruction.

>+  if (mArchive)
>+    delete mArchive;

'delete NULL;' is legal. But you want this to be an nsAutoPtr - no need for any manual deletes or construction with NULL!

>+NS_IMETHODIMP
>+StartupCache::Init() 
>+{
>+  nsresult rv;
>+  mTable.Init();
>+#ifdef DEBUG
>+  mWriteObjectMap.Init();
>+#endif
>+  mZipW = do_CreateInstance("@mozilla.org/zipwriter;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsCOMPtr<nsIFile> file;
>+  rv = NS_GetSpecialDirectory("ProfLDS",
>+                              getter_AddRefs(file));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = file->AppendNative(nsCAutoString("startupCache"));

NS_LITERAL_CSTRING

Also, I'd just make mFile an nsILocalFile and QI to it here -- fail hard if it doesn't work.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  // Try to create the directory if it's not there yet
>+  rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);
>+  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS)
>+    return rv;

More newlines please!

>+
>+  char* wordsize = sizeof(void*) == 4 ? "startupCache.4." : "startupCache.8.";

static const char*.

>+  nsCAutoString sizeStr(wordsize);
>+  nsCString endianStr(SC_ENDIAN);
>+  sizeStr.Append(endianStr);

You can build this entire string statically as a global:

#if PR_BYTES_PER_WORD == 4
#define SC_WORDSIZE "4"
#else
#define SC_WORDSIZE "8"
#endif

static const char* sStartupCacheName = "startupCache." SC_WORDSIZE "." SC_ENDIAN;

>+  rv = file->AppendNative(sizeStr);

NS_LITERAL_CSTRING(sStartupCacheName)

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  mFile = file; 
>+  mObserverService = do_GetService("@mozilla.org/observer-service;1");
>+  
>+  if (!mObserverService) {
>+    NS_WARNING("Could not get observerService.");
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+  
>+  mListener = new StartupCacheListener();  
>+  if (!mListener) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

Don't need this check.

>+  rv = mObserverService->AddObserver(mListener, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
>+                                     PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  rv = LoadArchive();
>+  
>+  // Sometimes we don't have a cache yet, that's ok.
>+  if (rv == NS_ERROR_FILE_NOT_FOUND)
>+    rv = NS_OK;
>+  // If it's corrupted, just remove it and start over.
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("Failed to load startupcache file correctly, removing!");
>+    InvalidateCache();
>+    rv = NS_OK;
>+  }

Simplify these branches?

if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) { ... }

>+
>+  mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  // Wait for 10 seconds, then write out the cache.
>+  // Changed to 1 sec for testing, change this back!
>+  rv = mTimer->InitWithFuncCallback(StartupCache::Timeout, this, 1000,
>+                                    nsITimer::TYPE_ONE_SHOT);

Change this back? ;)

>+nsresult
>+StartupCache::LoadArchive() 
>+{
>+  PRBool exists;
>+  nsresult rv = mFile->Exists(&exists);
>+  if (NS_FAILED(rv) || !exists)
>+    return NS_ERROR_FILE_NOT_FOUND;
>+  
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mFile, &rv);
>+  if (NS_FAILED(rv))
>+    // maybe okay, but this is super weird...
>+    return NS_ERROR_FILE_NOT_FOUND;

Push this into ::Init(), per above

>+  
>+  if (mArchive)
>+    delete mArchive;

Can be 'mArchive = NULL' with an nsAutoPtr. You should move this to the top of the method, such that if you return NS_ERROR_FILE_NOT_FOUND, you don't end up in an inconsistent state.

>+// NOTE: this will not find a new entry until it has been written to disk!
>+NS_IMETHODIMP
>+StartupCache::GetBuffer(const char* id, char** outbuf, PRUint32* aLength)

Nit: you should pick an argument naming convention (either 'foo' or 'aFoo'), and stick with it. It's your module so up to you. :)

>+  nsresult rv = NS_ERROR_NOT_AVAILABLE;

Prefer you just 'return NS_ERROR_NOT_AVAILABLE' under that condition. Better for readability.

>+  PRBool exists;
>+
>+  if (!mStartupWriteInitiated) {
>+    CacheEntry* entry; 

const CacheEntry* (if Get() doesn't choke?)

>+    nsCString idStr(id);
>+    mTable.Get(idStr, &entry);

nsDependentCString(id)

>+    if (entry) {
>+      char* buf = new char[entry->size];
>+      if (!buf) {
>+        rv = NS_ERROR_OUT_OF_MEMORY;
>+      } else {
>+        memcpy(buf, entry->data, entry->size);
>+        *outbuf = buf;
>+        *aLength = entry->size;
>+        rv = NS_OK;
>+      }
>+      return rv;

No need for the error check. FWIW, I also prefer the style:

if (error)
  return NS_ERROR_...;

[success condition]

which results in less net indentation and thusly better readability.

>+    }
>+  }
>+
>+  if (mArchive) {
>+    nsZipItem* zipItem = mArchive->GetItem(id);
>+    if (zipItem) {
>+      PRUint8* itemData = mArchive->GetData(zipItem);
>+      
>+      if (!mArchive->CheckCRC(zipItem, itemData)) {

Let's nullcheck itemData here and handle failure before calling CheckCRC.

>+        NS_WARNING("StartupCache file corrupted!");
>+        InvalidateCache();
>+        return NS_ERROR_FILE_CORRUPTED;
>+      }
>+
>+      rv = NS_ERROR_OUT_OF_MEMORY;
>+      PRUint32 size = zipItem->Size();
>+      char* buf = new char[size];
>+      if (buf) {

No need.

>+        memcpy(buf, itemData, size);
>+        *outbuf = buf;
>+        *aLength = size;
>+        rv = NS_OK;

return NS_OK;

>+      }
>+    }
>+  }
>+  
>+  return rv;
>+}
>+
>+NS_IMETHODIMP
>+StartupCache::PutBuffer(const char* id, char* inbuf, PRUint32 len) 
>+{

const char* inbuf

>+  nsresult rv;
>+
>+  if (StartupCache::gShutdownInitiated) {
>+    // XXX Better error code.
>+    return NS_ERROR_UNEXPECTED;

NS_ERROR_NOT_AVAILABLE?

>+  }
>+
>+  if (!mStartupWriteInitiated) {
>+    // Cache it for now, we'll write all together later.
>+    CacheEntry* entry; 
>+    nsCString idStr(id);
>+    mTable.Get(idStr, &entry);

nsDependentCString(id)

>+    NS_ASSERTION(entry == nsnull, "Existing entry in StartupCache.");
>+
>+    if (mArchive) {
>+      nsZipItem* zipItem = mArchive->GetItem(id);
>+      NS_ASSERTION(zipItem == nsnull, "Existing entry in disk StartupCache.");
>+    }
>+
>+    entry = new CacheEntry(inbuf, len);
>+    mTable.Put(idStr, entry);
>+    return NS_OK;
>+  }
>+  
>+  rv = mZipW->Open(mFile, PR_RDWR | PR_CREATE_FILE);
>+  NS_ENSURE_SUCCESS(rv, rv);  
>+
>+  // XXX We need to think about whether to write this out every time,
>+  // or somehow detect a good time to write.  We need to finish writing
>+  // before shutdown though, and writing also requires a reload of the
>+  // reader's archive, which probably can't handle having the underlying
>+  // file change underneath it. Potentially could reload on the next
>+  // read request, if this is a problem.

As discussed, we want asynchronous writes and post-startup batching. File a separate bug for that, and put down the ideas we settled on? I think that followup should be a high priority since we've been undertaking a major push for getting sync IO off main thread. Perhaps a blocker, but could even go in a minor release?

>+  nsCString key(id);
>+  PRBool hasEntry;
>+  rv = mZipW->HasEntry(key, &hasEntry);

nsDependentCString(id), but just share the one you already created above.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(hasEntry == PR_FALSE, "Existing entry in disk StartupCache.");

If you're just asserting this, then wrap the HasEntry code in an #ifdef DEBUG?

>+  
>+  nsCOMPtr<nsIStringInputStream> stream
>+    = do_CreateInstance("@mozilla.org/io/string-input-stream;1",
>+                        &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = stream->AdoptData(inbuf, len);
>+  NS_ENSURE_SUCCESS(rv, rv);

I don't see it documented clearly that PutBuffer adopts what's passed in. Also, I think 'new' and nsMemory::Free are mismatched allocators... you should have consumers use nsMemory::Alloc.

All this stream stuff is unfortunate. It'd be really nice if we could just pass around lightweight dynamically resizable buffers, a la nsTArray<char>&. :(

What's stopping us from doing that? You allude to it in comment 59 but I don't have enough context yet.

Note: you could avoid having the adoption semantic if you use nsIStringInputStream::ShareData here. I think that'd be preferable, since this method is synchronous right now anyway. When it becomes async, SC can copy the data into a temporary buffer for writing...

Speaking of mismatched allocators, I'd like you to run this whole patch + consumers through valgrind, to make sure you don't read uninitialized memory / go off the end of data / leak / mismatch allocators. It's super easy, I'll show you how!

>+  rv = mZipW->AddEntryStream(key, 0, 0, stream, false);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = mZipW->Close();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  // our reader's view of the archive is outdated now, reload it.
>+  if (mArchive) {
>+    delete mArchive;
>+    mArchive = NULL;
>+  }
>+  rv = LoadArchive();
>+  
>+  return rv;

return LoadArchive();

>+}
>+
>+PLDHashOperator
>+CacheCloseHelper(const nsACString& key, nsAutoPtr<CacheEntry>& data, 
>+                 void* closure) 
>+{
>+  nsresult rv;
>+  nsIZipWriter* writer = (nsIZipWriter*) closure;
>+  
>+  nsCOMPtr<nsIStringInputStream> stream
>+    = do_CreateInstance("@mozilla.org/io/string-input-stream;1", &rv);

Hm. Can you call ShareData() multiple times on a single stream? If so, could you make 'closure' a struct, and pull 'stream' out into it?

>+  if (rv == NS_OK) {

NS_SUCCEEDED(rv)

>+    stream->ShareData(data->data, data->size);
>+    writer->RemoveEntry(key, false);

Why do you need to RemoveEntry()? To make sure it doesn't already exist? Can we just assert that instead?

>+    writer->AddEntryStream(key, 0, 0, stream, false);
>+  } else {
>+    NS_WARNING("cache entry deleted but not written to disk.");
>+  }
>+  return PL_DHASH_REMOVE;
>+}
>+
>+NS_IMETHODIMP
>+StartupCache::WriteToDisk() 
>+{
>+  nsresult rv;
>+  if (mTable.Count() > 0) {
>+    rv = mZipW->Open(mFile, PR_RDWR | PR_CREATE_FILE);
>+    if (NS_FAILED(rv)) {
>+      NS_WARNING("could not open zipfile for write");
>+    } else {
>+      mTable.Enumerate(CacheCloseHelper, mZipW);
>+      mZipW->Close();
>+      
>+      // our reader's view of the archive is outdated now, reload it.
>+      if (mArchive) {
>+        delete mArchive;
>+        mArchive = NULL;
>+      }

LoadArchive() already does this for you.

>+      LoadArchive();
>+    }
>+  }
>+  
>+  return rv;

'rv' can be uninitialized here. You should look at your compiler warnings ;)

Why does this have to be virtual, or return non-void?

>+}
>+
>+NS_IMETHODIMP
>+StartupCache::InvalidateCache() 

Why virtual and non-void?

>+{
>+  NS_WARNING("Removing startupCache file, will not fastload on next start");

Is this sufficiently strange that we should warn about it?

>+  mTable.Clear();
>+  if (mArchive) {
>+    delete mArchive;
>+    mArchive = NULL;
>+  }

LoadArchive() does this for you.

>+  mFile->Remove(false);
>+  LoadArchive();
>+  return NS_OK;
>+}
>+
>+void
>+StartupCache::Timeout(nsITimer *aTimer, void *aClosure)

Could use a better name here. WriteTimeout?

>+{
>+  StartupCache* sc = (StartupCache*) aClosure;
>+  sc->mStartupWriteInitiated = PR_TRUE;
>+  nsresult rv = sc->WriteToDisk();

Why doesn't WriteToDisk() set mStartupWriteInitiated?

>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("Startup write failed.");
>+  }
>+}
>+
>+// We don't want to refcount StartupCache, so we'll just
>+// hold a strong ref to this and pass it to observerService instead.
>+// (ObserverService refcounts the objects we pass to it).
>+NS_IMPL_THREADSAFE_ISUPPORTS1(StartupCacheListener, nsIObserver)
>+
>+NS_IMETHODIMP
>+StartupCacheListener::Observe(nsISupports *subject, const char* topic, const PRUnichar* data)
>+{
>+  nsresult rv = NS_OK;
>+  if (strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID) == 0) {
>+    StartupCache::gShutdownInitiated = PR_TRUE;
>+  }/*
>+  if (strcmp(topic, NS_XPCOM_STARTUP_OBSERVER_ID) == 0) {

'else if'

Why is this code chunk commented out?

>+    nsCOMPtr<nsITimer> timer = do_CreateInstance("@mozilla.org/timer;1", &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    StartupCache* sc = StartupCache::GetSingleton();
>+    // Prevent future writes from going into the in-memory buffer.
>+    // XXX It actually seems that some pre-UI writes happen after this
>+    // point, and get written out individually. We may need to revisit this,
>+    // perhaps by flipping this bit in the Timeout instead of here. Could
>+    // create some additional complexity, esp. if we move IO to a different thread.
>+    sc->mStartupWriteInitiated = PR_TRUE;

Not sure why you want to stop memory batching at this point. Couldn't we let it happen naturally in Timeout()?

>+    if (sc->mTable.Count() > 0) {
>+      rv = timer->InitWithFuncCallback(StartupCache::Timeout, this, 3000,
>+                                       nsITimer::TYPE_ONE_SHOT);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      sc->mTimer = timer;
>+      }
>+      }*/
>+  return rv;
>+} 
>+
>+NS_IMETHODIMP
>+StartupCache::GetDebugObjectOutputStream(nsIObjectOutputStream* aStream,
>+                                         nsIObjectOutputStream** aOutStream) 
>+{
>+#ifdef DEBUG
>+  StartupCacheDebugOutputStream* stream
>+    = new StartupCacheDebugOutputStream(aStream, &mWriteObjectMap);

Can you check aStream for nullness here? Also, since &mWriteObjectMap can never be null, you shouldn't need to check it.

>+  if (!stream)
>+    return NS_ERROR_OUT_OF_MEMORY;

Don't need this.

>+  NS_ADDREF(*aOutStream = stream);
>+#else
>+  NS_ADDREF(*aOutStream = aStream);
>+#endif
>+  
>+  return NS_OK;
>+}
>+
>+// StartupCacheDebugOutputStream implementation
>+#ifdef DEBUG
>+NS_IMPL_ISUPPORTS3(StartupCacheDebugOutputStream, nsIObjectOutputStream, 
>+                   nsIBinaryOutputStream, nsIOutputStream)
>+
>+nsresult
>+StartupCacheDebugOutputStream::CheckReferences(nsISupports* aObject)
>+{
>+  nsresult rv;
>+  if (!mBinaryStream || !mObjectMap)
>+    return NS_ERROR_NULL_POINTER;

Per above -- no need for nullchecks here if you push them into GetDebugObjectOutputStream(). Same in all the other methods where you check them.

>+  
>+  nsCOMPtr<nsIClassInfo> classInfo = do_QueryInterface(aObject);
>+  if (!classInfo) {
>+    NS_NOTREACHED("aObject must implement nsIClassInfo");

Mm, NS_ERROR(...) sounds better here.

>+    return NS_ERROR_FAILURE;
>+  }
>+  
>+  PRUint32 flags;
>+  rv = classInfo->GetFlags(&flags);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (flags & nsIClassInfo::SINGLETON)
>+    return NS_OK;
>+  
>+  nsISupportsHashKey* key = mObjectMap->GetEntry(aObject);
>+  if (key) {
>+    NS_NOTREACHED("non-singleton aObject is referenced multiple times in this" 
>+                  "serialization, we don't support that.");

NS_ERROR

>+    return NS_ERROR_FAILURE;
>+  }
>+  if (!mObjectMap->PutEntry(aObject))
>+    return NS_ERROR_OUT_OF_MEMORY;

No need.

>+  
>+  return NS_OK;
>+}
>+
>+// nsIObjectOutputStream implementation
>+NS_IMETHODIMP
>+StartupCacheDebugOutputStream::WriteObject(nsISupports* aObject, PRBool aIsStrongRef)
>+{
>+  nsresult rv;
>+  
>+  nsCOMPtr<nsISupports> rootObject(do_QueryInterface(aObject));
>+  
>+  NS_ASSERTION(rootObject.get() == aObject,
>+               "bad call to WriteObject -- call WriteCompoundObject!");
>+  rv = CheckReferences(aObject);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return mBinaryStream->WriteObject(aObject, aIsStrongRef);
>+}
>+
>+NS_IMETHODIMP
>+StartupCacheDebugOutputStream::WriteSingleRefObject(nsISupports* aObject)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsISupports> rootObject(do_QueryInterface(aObject));
>+  
>+  NS_ASSERTION(rootObject.get() == aObject,
>+               "bad call to WriteSingleRefObject -- call WriteCompoundObject!");
>+  rv = CheckReferences(aObject);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return mBinaryStream->WriteSingleRefObject(aObject);
>+}
>+
>+NS_IMETHODIMP
>+StartupCacheDebugOutputStream::WriteCompoundObject(nsISupports* aObject,
>+                                                const nsIID& aIID,
>+                                                PRBool aIsStrongRef)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsISupports> rootObject(do_QueryInterface(aObject));
>+  
>+  nsCOMPtr<nsISupports> roundtrip;
>+  rootObject->QueryInterface(aIID, getter_AddRefs(roundtrip));
>+  NS_ASSERTION(roundtrip.get() == aObject,
>+               "bad aggregation or multiple inheritance detected by call to "
>+               "WriteCompoundObject!");

Will this catch all cases of multiple inheritance? It seems like it could happen to roundtrip back to the same pointer in some instances, if the incoming pointer happens to be on the same branch. But there's not much you can do to detect that, right?

>+StartupCacheWrapper* StartupCacheWrapper::GetSingleton() 
>+{
>+  if (gStartupCacheWrapper) {
>+    NS_ADDREF(gStartupCacheWrapper);
>+    return gStartupCacheWrapper;
>+  }    
>+  gStartupCacheWrapper = new StartupCacheWrapper();
>+  NS_IF_ADDREF(gStartupCacheWrapper);

NS_ADDREF

>diff --git a/startupcache/StartupCache.h b/startupcache/StartupCache.h

>+#ifndef StartupCache_h_
>+#define StartupCache_h_
>+
>+#include "prio.h"
>+#include "prtypes.h"
>+
>+#include "nsISupports.h"

Shouldn't need to #include this, other nsI*'s will do it for you.

>+#include "nsClassHashtable.h"
>+#include "nsIZipWriter.h"
>+#include "nsIZipReader.h"
>+#include "nsComponentManagerUtils.h"
>+#include "nsZipArchive.h"
>+#include "nsIStartupCache.h"
>+#include "nsIStorageStream.h"
>+#include "nsITimer.h"
>+#include "nsIObserverService.h"
>+#include "nsIObserver.h"
>+#include "nsIOutputStream.h"
>+#include "nsIFile.h"
>+#include "pldhash.h"

Shouldn't need pldhash.h either?

>+
>+#include "nsDebug.h"

Or this!

>+
>+static NS_DEFINE_CID(kZipReaderCID, NS_ZIPREADER_CID);

This should go in the cpp.

>+
>+/**
>+ * The StartupCache is a persistent cache of simple key-value pairs,
>+ * where the keys are null-terminated c-strings and the values are 
>+ * arbitrary data, passed as a (char*, size) tuple. 
>+ *
>+ * Clients should use the GetSingleton() static method to access the cache. It 
>+ * will be available from the end of XPCOM init (NS_InitXPCOM3 in nsXPComInit.cpp), 
>+ * until XPCOM shutdown begins. The GetSingleton() method will return null if the cache
>+ * is unavailable. The cache is only provided for libxul builds --
>+ * it will fail to link in non-libxul builds. The XPCOM interface is provided
>+ * only to allow compiled-code tests; clients should avoid using it.
>+ *
>+ * The API provided is very simple: GetBuffer() returns a buffer that was previously
>+ * stored in the cache (if any), and PutBuffer() inserts a buffer into the cache.
>+ * PutBuffer will assert if client attempts to insert a buffer with the same name as

* if the client

>+ * an existing entry. PutInputStream provides the same functionality as PutBuffer,

Semicolon instead of comma.

>+ * the resulting buffer stored should be the result of calling Read() on the stream
>+ * with maximum length. The API does not provide a GetInputStream call, to avoid confusion
>+ * over what happens when two inputstreams are requested for the same id. Clients can
>+ * simply wrap the result of GetBuffer with a StringInputStream, or using the provided
>+ * utility functions (see below).
>+ *
>+ * InvalidateCache() may be called if a client suspects data corruption 
>+ * or wishes to invalidate for any other reason. This will remove all existing cache data.
>+ * Finally, getDebugObjectOutputStream() allows debug code to wrap an objectstream
>+ * with a debug objectstream, to check for multiply-referenced objects. These will
>+ * generally fail to deserialize correctly, unless they are stateless singletons or the 
>+ * client maintains their own object data map for deserialization.
>+ *
>+ * Writes before the final-ui-startup notification are placed in an intermediate
>+ * cache in memory, then written out to disk at a later time, to get writes off the
>+ * startup path. In any case, clients should not rely on being able to GetBuffer()
>+ * data that is written to the cache, since it may not have been written to disk or
>+ * another client may have invalidated the cache. In other words, it should be used as
>+ * a cache only, and not a reliable persistent store.
>+ *
>+ * Some utility functions are provided in StartupCacheUtils. These functions wrap the
>+ * buffers into object streams, which may be useful for serializing objects. Note
>+ * the above caution about multiply-referenced objects, though -- the streams are just
>+ * as 'dumb' as the underlying buffers about multiply-referenced objects. They just
>+ * provide some convenience in writing out data.
>+ */

A++ would read again!

>+
>+namespace mozilla {
>+namespace scache {
>+
>+struct CacheEntry 
>+{
>+  char* data;
>+  PRUint32 size;
>+
>+  CacheEntry() : data(nsnull), size(0) { }

Do you need this ctor?

>+
>+  // Takes possession of buf
>+  CacheEntry(char* buf, PRUint32 len) : data(buf), size(len) { }
>+
>+  ~CacheEntry()
>+  {
>+    if (data) {
>+      delete data;
>+    }

No need for the nullcheck.

Should use nsAutoArrayPtr<char> here, which will also avoid your 'new char[]' vs 'delete' mismatch.

>+  }
>+};
>+
>+// We don't want to refcount StartupCache, and ObserverService wants to
>+// refcount its listeners, so we'll let it refcount this instead.
>+class StartupCacheListener : public nsIObserver
>+{
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBSERVER
>+};
>+
>+class StartupCache
>+{
>+
>+friend class StartupCacheListener;
>+friend class StartupCacheWrapper;
>+                                
>+public:
>+  ~StartupCache();

This can be private.

>+
>+  nsresult GetBuffer(const char* id, char** outbuf, PRUint32* length);
>+  nsresult PutBuffer(const char* id, char* inbuf, PRUint32 length);
>+  nsresult InvalidateCache();
>+  nsresult GetDebugObjectOutputStream(nsIObjectOutputStream* aStream,
>+                                      nsIObjectOutputStream** outStream);
>+
>+  static StartupCache* GetSingleton();
>+
>+  // Only should be called once per run, typically in InitXPCOM
>+  static nsresult InitSingleton();

Fold this into GetSingleton()?

>+#ifdef DEBUG
>+  nsTHashtable<nsISupportsHashKey> mWriteObjectMap;
>+#endif

Why public?

>+// This debug outputstream attempts to detect if clients are writing multiple
>+// references to the same object. We only support that if that object
>+// is a singleton.
>+#ifdef DEBUG
>+class StartupCacheDebugOutputStream
>+  : public nsIObjectOutputStream
>+{  
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIOBJECTOUTPUTSTREAM
>+
>+  StartupCacheDebugOutputStream (nsIObjectOutputStream* binaryStream,
>+                                   nsTHashtable<nsISupportsHashKey>* objectMap)
>+  : mBinaryStream(binaryStream), mObjectMap(objectMap) { }
>+  
>+  NS_FORWARD_SAFE_NSIBINARYOUTPUTSTREAM(mBinaryStream)
>+  NS_FORWARD_SAFE_NSIOUTPUTSTREAM(mBinaryStream)
>+  
>+  nsresult CheckReferences(nsISupports* aObject);

Use bool return type?

>+  nsCOMPtr<nsIObjectOutputStream> mBinaryStream;
>+  nsTHashtable<nsISupportsHashKey> *mObjectMap;
>+};
>+#endif // DEBUG
>+
>+// XPCOM wrapper interface provided for tests only.
>+#define NS_STARTUPCACHE_CID \
>+      {0xae4505a9, 0x87ab, 0x477c, \
>+      {0xb5, 0x77, 0xf9, 0x23, 0x57, 0xed, 0xa8, 0x84}}
>+// contract id: "@mozilla.org/startupcache/cache;1"
>+
>+class StartupCacheWrapper 
>+  : public nsIStartupCache
>+{
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSISTARTUPCACHE
>+
>+  static StartupCacheWrapper* GetSingleton();
>+  static StartupCacheWrapper *gStartupCacheWrapper;
>+};

Hmm. You could #ifdef ENABLE_TESTS this whole monster?

>+
>+} // namespace scache
>+} // namespace mozilla
>+#endif //StartupCache_h_

>diff --git a/startupcache/StartupCacheModule.cpp b/startupcache/StartupCacheModule.cpp

>+ * Contributor(s):
>+ *   Daniel Veditz <dveditz@netscape.com>

You go by many names. :)

>diff --git a/startupcache/StartupCacheUtils.h b/startupcache/StartupCacheUtils.h

>+inline nsresult
>+NS_NewObjectInputStreamFromBuffer(char* buffer, PRUint32 len, 
>+                                  nsIObjectInputStream** stream)
>+{
>+  nsCOMPtr<nsIStringInputStream> stringStream
>+  = do_CreateInstance("@mozilla.org/io/string-input-stream;1");

Add a double-space before the =, and elsewhere.

>+  if (!stringStream)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  nsCOMPtr<nsIObjectInputStream> objectInput 
>+  = do_CreateInstance("@mozilla.org/binaryinputstream;1");
>+  if (!objectInput)
>+    return NS_ERROR_OUT_OF_MEMORY;

I think you can ditch these nullchecks, and just unconditionally return NS_OK.

>+  
>+  stringStream->AdoptData(buffer, len);
>+  objectInput->SetInputStream(stringStream);
>+  
>+  NS_ADDREF(*stream = objectInput);

objectInput.forget(stream), ditto elsewhere you use NS_ADDREF for outparams.

>+  return NS_OK;
>+}
>+
>+// We can't retrieve the wrapped stream from the objectOutputStream later,
>+// so we return it here.
>+inline nsresult

As above, I think you can ditch all the error checks.

>+NS_NewObjectOutputWrappedStorageStream(nsIObjectOutputStream **wrapperStream,
>+                                       nsIStorageStream** stream)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIStorageStream> storageStream
>+    = do_CreateInstance("@mozilla.org/storagestream;1");
>+  NS_ENSURE_ARG_POINTER(storageStream);

NS_ENSURE_ARG_POINTER is used for checking pointer inparams -- you'd want to use NS_ENSURE_TRUE here.

>+  
>+  rv = storageStream->Init(256, (PRUint32) -1, nsnull);

Use PR_UINT32_MAX instead of -1.

>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsIObjectOutputStream> objectOutput
>+  = do_CreateInstance("@mozilla.org/binaryoutputstream;1");
>+  if (!objectOutput)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  
>+  nsCOMPtr<nsIOutputStream> outputStream
>+    = do_QueryInterface(storageStream);
>+  
>+  objectOutput->SetOutputStream(outputStream);
>+  
>+#ifdef DEBUG
>+  // Wrap in debug stream to detect unsupported writes of 
>+  // multiply-referenced non-singleton objects
>+  StartupCache* sc = StartupCache::GetSingleton();
>+  NS_ENSURE_ARG_POINTER(sc);
>+  nsCOMPtr<nsIObjectOutputStream> debugStream;
>+  sc->GetDebugObjectOutputStream(objectOutput, getter_AddRefs(debugStream));
>+  NS_ADDREF(*wrapperStream = debugStream);
>+#else
>+  NS_ADDREF(*wrapperStream = objectOutput);
>+#endif
>+  
>+  NS_ADDREF(*stream = storageStream);
>+  return NS_OK;
>+}
>+
>+inline nsresult
>+NS_NewBufferFromStorageStream(nsIStorageStream *storageStream, 
>+                              char** buffer, PRUint32* len) 
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIInputStream> inputStream;
>+  rv = storageStream->NewInputStream(0, getter_AddRefs(inputStream));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  PRUint32 avail, read;
>+  rv = inputStream->Available(&avail);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  char* temp = new char[avail];

nsAutoArrayPtr<char>

>+  if (!temp)
>+    return NS_ERROR_OUT_OF_MEMORY;

No need for the nullcheck.

>+  
>+  rv = inputStream->Read(temp, avail, &read);
>+  if (NS_SUCCEEDED(rv) && avail != read)
>+    rv = NS_ERROR_UNEXPECTED;
>+  
>+  if (NS_FAILED(rv)) {
>+    delete temp;

Ditch the delete.

>+    return rv;
>+  }
>+  
>+  *len = avail;
>+  *buffer = temp;

*buffer = temp.forget();

>diff --git a/startupcache/nsIStartupCache.idl b/startupcache/nsIStartupCache.idl

>+[scriptable, uuid(de798fab-af49-4a61-8144-81550986e1da)]
>+interface nsIStartupCache : nsISupports
>+{
>+  /** Caller takes ownership of the resulting buffer. */
>+  [noscript] PRUint32 getBuffer(in string aID, out charPtr aBuffer);
>+  [noscript] void putBuffer(in string aID, in charPtr aBuffer, 
>+                            in PRUint32 aLength);
>+  void invalidateCache();

Need more comments here.

>diff --git a/startupcache/nsStartupCacheUtils.cpp b/startupcache/nsStartupCacheUtils.cpp

>+nsresult
>+NS_NewObjectInputStreamFromBuffer(char* buffer, int len, 
>+                                  nsIObjectInputStream** stream)
>+{
>+  nsCOMPtr<nsIStringInputStream> stringStream
>+    = do_CreateInstance("@mozilla.org/io/string-input-stream;1");
>+  if (!stringStream)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  nsCOMPtr<nsIObjectInputStream> objectInput 
>+    = do_CreateInstance("@mozilla.org/binaryinputstream;1");
>+  if (!objectInput)
>+    return NS_ERROR_OUT_OF_MEMORY;

Don't need the nullchecks.

>+
>+  stringStream->AdoptData(buffer, len);
>+  objectInput->SetInputStream(stringStream);
>+
>+  NS_ADDREF(*stream = objectInput);

.forget() trick please, ditto below!

>+  return NS_OK;
>+}
>+
>+// This is questionable API name and design, but we can't
>+// retrieve the wrapped stream from the objectOutputStream later...
>+nsresult
>+NS_NewObjectOutputWrappedStorageStream(nsIObjectOutputStream **wrapperStream,
>+                                       nsIStorageStream** stream)
>+{
>+  nsCOMPtr<nsIStorageStream> storageStream;
>+  nsresult rv = NS_NewStorageStream(256, (PRUint32)-1, 

PR_UINT32_MAX?

>+                                    getter_AddRefs(storageStream));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIObjectOutputStream> objectOutput
>+    = do_CreateInstance("@mozilla.org/binaryoutputstream;1");
>+  if (!objectOutput)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  nsCOMPtr<nsIOutputStream> outputStream
>+    = do_QueryInterface(storageStream);
>+
>+  objectOutput->SetOutputStream(outputStream);
>+  NS_ADDREF(*wrapperStream = objectOutput);
>+  NS_ADDREF(*stream = storageStream);
>+  return NS_OK;
>+}
>+
>+nsresult
>+NS_NewBufferFromStorageStream(nsIStorageStream *storageStream, 
>+                              char** buffer, int* len)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIInputStream> inputStream;
>+  rv = storageStream->NewInputStream(0, getter_AddRefs(inputStream));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 avail, read;
>+  rv = inputStream->Available(&avail);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  char* temp = new char[avail];

Choose a better name than 'temp' :)

Also nsAutoArrayPtr<char>, and then...

>+  if (!temp)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  rv = inputStream->Read(temp, avail, &read);
>+  if (NS_SUCCEEDED(rv) && avail != read)
>+    rv = NS_ERROR_UNEXPECTED;
>+
>+  if (NS_FAILED(rv)) {
>+    delete temp;
>+    return rv;
>+  }
>+
>+  *len = avail;
>+  *buffer = temp;

*buffer = temp.forget();

>+  return NS_OK;
>+}
>+

>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in

>@@ -46,16 +46,17 @@
> include $(topsrcdir)/rdf/util/src/objs.mk
> endif
> include $(topsrcdir)/intl/unicharutil/util/objs.mk
> 
> MODULE = libxul
> LIBRARY_NAME = xul
> FORCE_USE_PIC = 1
> FORCE_SHARED_LIB = 1
>+#FORCE_STATIC_LIB = 1

Remove.

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>@@ -2464,16 +2464,19 @@
>   if (!file)
>     return;
> 
>   file->AppendNative(NS_LITERAL_CSTRING("XUL" PLATFORM_FASL_SUFFIX));
>   file->Remove(PR_FALSE);
>   
>   file->SetNativeLeafName(NS_LITERAL_CSTRING("XPC" PLATFORM_FASL_SUFFIX));
>   file->Remove(PR_FALSE);
>+
>+  file->SetNativeLeafName(NS_LITERAL_CSTRING("startupCache"));
>+  file->Remove(PR_TRUE);

Wait, isn't the file 'startupCache.4.big' or something?

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp

>@@ -682,21 +686,23 @@
>         nsComponentManagerImpl::gComponentManager->AutoRegister(nsnull);
>     }
> 
>     // After autoreg, but before we actually instantiate any components,
>     // add any services listed in the "xpcom-directory-providers" category
>     // to the directory service.
>     nsDirectoryService::gService->RegisterCategoryProviders();
> 
>+#ifdef MOZ_ENABLE_LIBXUL
>+    mozilla::scache::StartupCache::InitSingleton();

If you folded InitSingleton into GetSingleton, this could just be the latter.

>@@ -757,17 +763,21 @@
>             {
>                 (void) observerService->
>                     NotifyObservers(mgr, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
>                                     nsnull);
>             }
>         }
> 
>         NS_ProcessPendingEvents(thread);
>-
>+#ifdef MOZ_ENABLE_LIBXUL
>+        mozilla::scache::StartupCache* sc 
>+            = mozilla::scache::StartupCache::GetSingleton();
>+        delete sc;

Maybe fold this into a DestroySingleton function, so consumers don't need to
know how it was created?

>diff --git a/xpcom/tests/TestHarness.h b/xpcom/tests/TestHarness.h

>@@ -322,17 +322,18 @@
>       if (mDirSvcProvider &&
>           NS_SUCCEEDED(mDirSvcProvider->GetFile(aProperty, _persistent,
>                                                 _result))) {
>         return NS_OK;
>       }
> 
>       // Otherwise, the test harness provides some directories automatically.
>       if (0 == strcmp(aProperty, NS_APP_USER_PROFILE_50_DIR) ||
>-          0 == strcmp(aProperty, NS_APP_USER_PROFILE_LOCAL_50_DIR)) {
>+          0 == strcmp(aProperty, NS_APP_USER_PROFILE_LOCAL_50_DIR) ||
>+          0 == strcmp(aProperty, "ProfLDS")) {

Can you use NS_APP_PROFILE_LOCAL_DIR_STARTUP instead of "ProfLDS" here?

I'll review the tests separately, so you can get started on the comments here.

r- so I can give it another look.

Also, if you're worried about review latency on the consumer patches, I could potentially review them -- provided the current reviewers are happy with that. I don't know the consumer code, but I'm fine with taking the time to crash-course myself in it if it saves you time -- and I'm sure the current reviewers can make the judgment on whether that's OK. ;)
Attachment #444790 - Flags: review?(dwitte) → review-
Comment on attachment 442521 [details] [diff] [review]
client mozJSComponentLoader

Yoink!
Attachment #442521 - Flags: review?(brendan) → review?(dwitte)
Just to help w/ review.
Attachment #442524 - Attachment is obsolete: true
Attachment #464594 - Flags: review?(jonas)
Attachment #442524 - Flags: review?(jonas)
Attachment #464594 - Attachment is patch: true
Attachment #464594 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 444790 [details] [diff] [review]
startup cache only, better test

Here's the test review.

>diff --git a/startupcache/test/TestStartupCache.cpp b/startupcache/test/TestStartupCache.cpp

>+nsZipArchive::nsZipArchive() {}
>+nsZipArchive::~nsZipArchive() {}
>+nsrefcnt nsZipHandle::Release() { return 1; }

What are these for?

>+#define NS_ENSURE_STR_MATCH(str1, str2, testname)  \
>+PR_BEGIN_MACRO                                     \
>+if (0 != strcmp(str1, str2)) {                     \
>+fail("failed " testname);                          \
>+return NS_ERROR_FAILURE;                           \
>+}                                                  \
>+passed("passed " testname);                        \
>+PR_END_MACRO

Indentation plz!

>+nsresult
>+TestCreateClose() {

What does this mean? It looks like you're just instantiating scache, not closing it.

>+  nsCOMPtr<nsIStartupCache> sc 
>+    = do_GetService("@mozilla.org/startupcache/cache;1");
>+  if (!sc) {
>+    fail("didn't get a pointer...");
>+  } else {
>+    passed("got a pointer?");
>+  }
>+  return NS_OK;
>+}
>+
>+// XXX This test won't work until we write on a seperate thread.

'separate'

>+nsresult
>+TestStartupWriteRead() {

Hmm. But main() claims to run this test?

>+  nsresult rv;
>+  nsCOMPtr<nsIStartupCache> sc 
>+    = do_GetService("@mozilla.org/startupcache/cache;1", &rv);
>+  
>+  char* str = "Market opportunities for BeardBook";
>+  char* buf = new char[strlen(str) + 1];
>+  strcpy(buf, str);
>+  char* id = "id";
>+  char* outbuf;
>+  PRUint32 len;
>+  rv = sc->InvalidateCache();
>+  NS_ENSURE_SUCCESS(rv, rv);

Can you also test putting something into the cache, invalidating it, and verifying that it's empty?

Also, the cache is assumed to be empty on entering this method, right? Since you're throwing "xpcom-startup" at it later. So no need to invalidate here?

>+
>+  rv = sc->PutBuffer(id, buf, strlen(buf) + 1);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsIObserverService> obs 
>+    = do_GetService("@mozilla.org/observer-service;1", &rv);
>+  rv = obs->NotifyObservers(nsnull, "xpcom-startup", nsnull);
>+  PR_Sleep(10 * PR_TicksPerSecond());

GetBuffer() will hit the in-memory table if !mStartupWriteInitiated. So could you test that GetBuffer() behaves as expected both before and after the startup write occurs?

>+  
>+  PRBool complete;
>+  while (true) {
>+    NS_ProcessPendingEvents(nsnull);
>+    rv = sc->StartupWriteComplete(&complete);
>+    if (complete || NS_FAILED(rv))

You should check NS_FAILED(rv) before complete.

>+      break;
>+    PR_Sleep(1 * PR_TicksPerSecond());
>+  }
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = sc->GetBuffer(id, &outbuf, &len);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_STR_MATCH(str, outbuf, "simple write/read");
>+  return NS_OK;
>+}

It would also be nice to test that "xpcom-startup" + PutBuffer() + "xpcom-shutdown" + "xpcom-startup" + GetBuffer() works. Both with and without a 10 second delay between startup and shutdown, to make sure that shutting down with data in the in-memory table works properly.

>+nsresult
>+TestWriteObject() {
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIURI> obj
>+    = do_CreateInstance("@mozilla.org/network/simple-uri;1");
>+  if (!obj) {
>+    fail("did not create object in test write object");
>+    return NS_ERROR_UNEXPECTED;
>+  }
>+  NS_NAMED_LITERAL_CSTRING(spec, "http://www.mozilla.org");
>+  obj->SetSpec(spec);
>+  nsCOMPtr<nsIStartupCache> sc = do_GetService("@mozilla.org/startupcache/cache;1", &rv);
>+
>+  rv = sc->InvalidateCache();
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // Create an object stream. Usually this is done with
>+  // NS_NewObjectOutputWrappedStorageStream, but that uses
>+  // StartupCache::GetSingleton in debug builds, and we
>+  // don't have access to that here. Obviously.

Hmm. Why are the NS_NewObjectOutputWrappedStorageStream etc functions inline? You could solve this by out-of-lining them in StartupCacheUtils.cpp and exporting them, right?

>+  char* id = "id";
>+  nsCOMPtr<nsIStorageStream> storageStream
>+    = do_CreateInstance("@mozilla.org/storagestream;1");
>+  NS_ENSURE_ARG_POINTER(storageStream);
>+  
>+  rv = storageStream->Init(256, (PRUint32) -1, nsnull);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsIObjectOutputStream> objectOutput
>+    = do_CreateInstance("@mozilla.org/binaryoutputstream;1");
>+  if (!objectOutput)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  
>+  nsCOMPtr<nsIOutputStream> outputStream
>+    = do_QueryInterface(storageStream);
>+  
>+  rv = objectOutput->SetOutputStream(outputStream);
>+
>+  if (NS_FAILED(rv)) {
>+    fail("failed to create output stream");
>+    return rv;
>+  }
>+  nsCOMPtr<nsISupports> objQI(do_QueryInterface(obj));
>+  rv |= objectOutput->WriteObject(objQI, PR_TRUE);

Why |=? Though it's a nop here, in general prefer that you check rv each time it's assigned, especially if you're returning the resulting code since |= will make it nonsensical.

>+int main(int argc, char** argv)
>+{
>+  int rv = 0;
>+  nsresult rv2;
>+  ScopedXPCOM xpcom("Startup Cache");
>+  
>+  if (NS_FAILED(TestCreateClose()))
>+    rv = 1;
>+  /*
>+  // Startup writes are delayed by a timer, so tests won't work
>+  // until we kick them onto a different thread (the write will
>+  // always come after execution of this function finishes).
>+  // Just skip the startup step for now, by calling this with nothing 
>+  // in the cache yet. Later, we will call TestStartupWrite here.

Even when it's async, you'll still need some event loop spinnage here: the message about the write completing will be posted back to the main thread's event queue. Which you're already doing in TestStartupWriteRead, no?

In general, no commented-out code please -- just push adding it into a followup bug (and file said bug!).
oops, diff should be correct now.
Attachment #464594 - Attachment is obsolete: true
Attachment #464599 - Flags: review?(jonas)
Attachment #464594 - Flags: review?(jonas)
Comment on attachment 464599 [details] [diff] [review]
client scXULCache, function names in diff

> nsXULPrototypeScript::DeserializeOutOfLine(nsIObjectInputStream* aInput,
>                                            nsIScriptGlobalObject* aGlobal)
> {
>     // Keep track of FastLoad failure via rv, so we can
>     // AbortFastLoads if things look bad.
>-    nsresult rv = NS_OK;
>-
>+    nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
>+#ifdef MOZ_ENABLE_LIBXUL
>+    rv = NS_OK;
>     nsXULPrototypeCache* cache = nsXULPrototypeCache::GetInstance();
>-    nsIFastLoadService* fastLoadService = cache->GetFastLoadService();
>-
>+  
>     // Allow callers to pass null for aInput, meaning
>     // "use the FastLoad service's default input stream."
>     // See nsXULContentSink.cpp for one use of this.
>+  
>+    // XXX I don't think we need this anymore
>     nsCOMPtr<nsIObjectInputStream> objectInput = aInput;
>-    if (! objectInput && fastLoadService)
>-        fastLoadService->GetInputStream(getter_AddRefs(objectInput));
>-
>-    if (objectInput) {
>+    
>+    if (cache) {

I'm not sure what the XXX comment is about. If it's about the code you're removing, just remove the comment. If it's about some other code, please clarify what code it's referring to (or attempt to remove said code along with the comment).

>@@ -3063,75 +3041,58 @@ nsXULPrototypeScript::DeserializeOutOfLi
>                 }
>             }
>         }
> 
>         if (! mScriptObject.mObject) {
>             nsCOMPtr<nsIURI> oldURI;
> 
>             if (mSrcURI) {
>-                nsCAutoString spec;
>-                mSrcURI->GetAsciiSpec(spec);
>-                rv = fastLoadService->StartMuxedDocument(mSrcURI, spec.get(),
>-                                                         nsIFastLoadService::NS_FASTLOAD_READ);
>-                if (NS_SUCCEEDED(rv))
>-                    rv = fastLoadService->SelectMuxedDocument(mSrcURI, getter_AddRefs(oldURI));
>+                nsCOMPtr<nsIInputStream> is;
>+                rv = cache->GetInputStream(mSrcURI, getter_AddRefs(objectInput));

|is| appears to be unused.

>             } else {
>                 // An inline script: check FastLoad multiplexing direction
>                 // and skip Deserialize if we're not reading from a
>                 // muxed stream to get inline objects that are contained in
>                 // the current document.
>-                PRInt32 direction;
>-                fastLoadService->GetDirection(&direction);
>-                if (direction != nsIFastLoadService::NS_FASTLOAD_READ)
>-                    rv = NS_ERROR_NOT_AVAILABLE;
>+                NS_WARNING("inline script in nsXULElement, not sure about how to"
>+                           "handle this but should be ok?");
>             }

Don't we have lots of XUL with inline scripts? Doesn't seem good to warn for all of those. Either the code works and there is no need to warn, or it doesn't and needs to be fixed?

Or am I wrong and we generally don't have inline scripts in XUL?

I really don't know this code, so had a hard time reviewing for correctness. Things looked fine as far as I was able to tell though.

r=me with that fixed.
Attachment #464599 - Flags: review?(jonas) → review+
Comment on attachment 442521 [details] [diff] [review]
client mozJSComponentLoader

>diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp

>+#include "nsIStringStream.h"
>+
>+#ifdef MOZ_ENABLE_LIBXUL
>+#include "mozilla/scache/StartupCache.h"
>+#include "mozilla/scache/StartupCacheUtils.h"
>+#endif
>+
> #if defined(MOZ_SHARK) || defined(MOZ_CALLGRIND) || defined(MOZ_VTUNE) || defined(MOZ_TRACEVIS)
> #include "jsdbgapi.h"
> #endif
> 
>+#include "nsIStorageStream.h"

Can this go up with the other #include?

> nsresult
> mozJSComponentLoader::ReallyInit()
> {
>+    
>     nsresult rv;
>-

o_O

> nsresult
>-mozJSComponentLoader::StartFastLoad(nsIFastLoadService *flSvc)
>+mozJSComponentLoader::ReadScript(StartupCache* cache,
>+                                 const char *nativePath, nsIURI *uri,
>+                                 JSContext *cx, JSScript **script)
> {
>-    if (!mFastLoadFile || !flSvc) {
>-        return NS_ERROR_NOT_AVAILABLE;
>+    nsresult rv;
>+  
>+    // XXX does the spec by itself uniquely identify?
>+    nsCAutoString spec;
>+    rv = uri->GetSpec(spec);

The existing flSvc has two tables; one for nsIURI's and one for native paths. God knows how the tables are related; it's clear that both are relevant to the operation of the cache however.

On the mozJSComponentLoader side, a little grepping around turned up http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1254. Which says that nativePath === uri->GetSpec() except in the XPCONNECT_STANDALONE case. mrbkap says he doesn't even think that builds anymore, and wishes it to die in a fire. So let's assume that the spec is the sole and unique key.

In which case ReadScript() doesn't even need a nativePath inparam.

You should also NS_SUCCEEDED(rv, rv) the GetSpec() call.

>+    char* buf;
>+    PRUint32 len;
>+    rv = cache->GetBuffer(spec.get(), &buf, &len);
>+    
>+    if (NS_FAILED(rv)) {
>+        return rv; // don't warn since NOT_AVAILABLE is an ok error.
>     }
>-
>-    // Now set our IO object as current, and create our streams.
>-    if (!mFastLoadIO) {
>-        mFastLoadIO = new nsXPCFastLoadIO(mFastLoadFile);
>-        NS_ENSURE_TRUE(mFastLoadIO, NS_ERROR_OUT_OF_MEMORY);
>-    }
>-
>-    nsresult rv = flSvc->SetFileIO(mFastLoadIO);
>+  
>+    nsCOMPtr<nsIObjectInputStream> ois;
>+    rv = NS_NewObjectInputStreamFromBuffer(buf, len, getter_AddRefs(ois));
>     NS_ENSURE_SUCCESS(rv, rv);

Early returns will drop 'buf' on the floor. You should make 'buf' an nsAutoArrayPtr. It'd really be much nicer if you used nsTArray<char> in this interface :(

>-nsresult
>-mozJSComponentLoader::WriteScript(nsIFastLoadService *flSvc, JSScript *script,
>+mozJSComponentLoader::WriteScript(StartupCache *cache, JSScript *script,
>                                   nsIFile *component, const char *nativePath,
>                                   nsIURI *uri, JSContext *cx)
> {
>-    NS_ASSERTION(flSvc, "fastload not initialized");
>     nsresult rv;
>-
>-    if (!mFastLoadOutput) {
>-        // Trying to read a URI that was not in the fastload file will have
>-        // created an output stream for us.  But, if we haven't tried to
>-        // load anything that was missing, it will still be null.
>-        rv = flSvc->GetOutputStream(getter_AddRefs(mFastLoadOutput));
>-        NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-
>-    NS_ASSERTION(mFastLoadOutput, "must have an output stream here");
>-
>-    LOG(("Writing %s to fastload\n", nativePath));
>-    rv = flSvc->AddDependency(component);
>+    nsCOMPtr<nsIObjectOutputStream> oos;
>+    nsCOMPtr<nsIStorageStream> storageStream; 
>+    rv = NS_NewObjectOutputWrappedStorageStream(getter_AddRefs(oos),
>+                                                getter_AddRefs(storageStream));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    rv = flSvc->StartMuxedDocument(uri, nativePath,
>-                                   nsIFastLoadService::NS_FASTLOAD_WRITE);
>+    rv = WriteScriptToStream(cx, script, oos);
>+    oos->Close();
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    nsCOMPtr<nsIURI> oldURI;
>-    rv = flSvc->SelectMuxedDocument(uri, getter_AddRefs(oldURI));
>+    nsCAutoString spec;
>+    rv = uri->GetSpec(spec);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    rv = WriteScriptToStream(cx, script, mFastLoadOutput);
>+    char* buf;
>+    PRUint32 len;
>+    rv |= NS_NewBufferFromStorageStream(storageStream, &buf, &len);

No |= plz?

>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    return flSvc->EndMuxedDocument(uri);
>+    cache->PutBuffer(spec.get(), buf, len);

The first thing that happens in PutBuffer() is an early return if gShutdownInitiated, which drops 'buf' on the floor. Either PutBuffer() unconditionally adopts, or it makes its own copy of the data.

> nsresult
> mozJSComponentLoader::GlobalForLocation(nsILocalFile *aComponent,
>                                         JSObject **aGlobal,
>                                         char **aLocation,
>                                         jsval *exception)
> {
>     nsresult rv;
>@@ -1174,71 +929,47 @@ mozJSComponentLoader::GlobalForLocation(
>     // XPCONNECT_STANDALONE case - but at least it builds and runs otherwise.
>     // See: http://bugzilla.mozilla.org/show_bug.cgi?id=121438
> #ifdef XPCONNECT_STANDALONE
>     localFile->GetNativePath(nativePath);
> #else
>     NS_GetURLSpecFromActualFile(aComponent, nativePath);
> #endif

Haha, this rotted!

> 
>+    nsCOMPtr<nsIURI> uri;
>+    rv = NS_NewURI(getter_AddRefs(uri), nativePath);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    JSScript *script = nsnull;

Thankfully, this isn't required anymore.

>+
>+#ifdef MOZ_ENABLE_LIBXUL  
>     // Before compiling the script, first check to see if we have it in
>     // the fastload file.  Note: as a rule, fastload errors are not fatal
>     // to loading the script, since we can always slow-load.
>-    nsCOMPtr<nsIFastLoadService> flSvc = do_GetFastLoadService(&rv);
> 
>-    // Save the old state and restore it upon return
>-    FastLoadStateHolder flState(flSvc);
>     PRBool fastLoading = PR_FALSE;
>-
>-    if (NS_SUCCEEDED(rv)) {
>-        rv = StartFastLoad(flSvc);
>-        if (NS_SUCCEEDED(rv)) {
>-            fastLoading = PR_TRUE;
>-        }
>-    }
>-
>-    nsCOMPtr<nsIURI> uri;
>-    rv = NS_NewURI(getter_AddRefs(uri), nativePath);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    JSScript *script = nsnull;
>+    StartupCache* cache = StartupCache::GetSingleton();
>+    if (cache)
>+        fastLoading = PR_TRUE;

'fastLoading' is both an anachronistic and confusing name. Can you change it? I think it really wants to mean 'writeToCache'...

> 
>     if (fastLoading) {

... and this really just wants to be 'cache'. 'writeToCache' could default to 'false' unless...

>-        rv = ReadScript(flSvc, nativePath.get(), uri, cx, &script);
>+        rv = ReadScript(cache, nativePath.get(), uri, cx, &script);
>         if (NS_SUCCEEDED(rv)) {
>             LOG(("Successfully loaded %s from fastload\n", nativePath.get()));
>             fastLoading = PR_FALSE; // no need to write out the script
>         } else if (rv == NS_ERROR_NOT_AVAILABLE) {
>             // This is ok, it just means the script is not yet in the
>             // fastload file.
>             rv = NS_OK;

... we hit this branch.

>         } else {
>-            LOG(("Failed to deserialize %s\n", nativePath.get()));
> 
>-            // Remove the fastload file, it may be corrupted.
>-            LOG(("Invalid fastload file detected, removing it\n"));
>-            nsCOMPtr<nsIObjectOutputStream> objectOutput;
>-            flSvc->GetOutputStream(getter_AddRefs(objectOutput));
>-            if (objectOutput) {
>-                flSvc->SetOutputStream(nsnull);
>-                objectOutput->Close();
>-            }
>-            nsCOMPtr<nsIObjectInputStream> objectInput;
>-            flSvc->GetInputStream(getter_AddRefs(objectInput));
>-            if (objectInput) {
>-                flSvc->SetInputStream(nsnull);
>-                objectInput->Close();
>-            }
>-            if (mFastLoadFile) {
>-                mFastLoadFile->Remove(PR_FALSE);
>-            }
>-            fastLoading = PR_FALSE;
>+          NS_ASSERTION(false, "need to remove this file if it's corrupt...");
>+          fastLoading = PR_FALSE;

GetBuffer() already does that for you, right? So no need to assert. In fact, no need for this entire branch since we know GetBuffer() will either succeed or return NS_ERROR_NOT_AVAILABLE. (Unless you keep the NS_ERROR_OUT_OF_MEMORY branch, in which case you should adjust appropriately here.)

Idle thought: people are going to have orphaned fastload files lying around. Should we add a couple lines in ReallyInit() to clean it up?

r- so I can take another look, but this looks pretty good. Love the code removal!
Attachment #442521 - Flags: review?(dwitte) → review-
Note to self: dwitte suggests that PutBuffer should copy (and not take ownership of the passed-in buffer).
Still hitting a crash in the test that I can't quite figure out, something to do with passing the nsIStringInputStream as part of the closure for CacheCloseHelper.

>Program received signal EXC_BAD_ACCESS, Could not access memory.
>Reason: KERN_INVALID_ADDRESS at address: 0x89005f01

If I do [p stream->ShareData("foo", 3)] in my debugger, it crashes again, so must be some problem with the way I'm passing the stream around.

Haven't added the other tests in yet, but other comments should be fixed. Hope to post patch with working and updated tests tomorrow, and will do line-by-line of review comments then.
Just to be clear, this is a crash that showed up tonight after changing things for review.
(In reply to comment #81)
> Just to be clear, this is a crash that showed up tonight after changing things
> for review.

I can help debug if you want!
>   mTable.Enumerate(CacheCloseHelper, mZipW);

That looks wrong.  Shouldn't the last arg be holder.get() or some such?
bz, yep. Was a bit embarrassed when I found that this morning...

Fixed comments as suggested, except these:

>const CacheEntry* (if Get() doesn't choke?)
It does choke :(

>I don't see it documented clearly that PutBuffer adopts what's passed in. Also,
>I think 'new' and nsMemory::Free are mismatched allocators... you should have
>consumers use nsMemory::Alloc.
Per discussion, I changed PutBuffer so that it makes a copy. 

>Why is this code chunk commented out?
After earlier discussion with bsmedberg and gavin, didn't seem like waiting for the startup event was really useful, because that event didn't really signal 'ui done loaded, won't interfere with startup'. There wasn't a 'right event' to listen for, so might as well just kick off a (long) timer right when the cache is initialized.

>Will this catch all cases of multiple inheritance? It seems like it could
>happen to roundtrip back to the same pointer in some instances, if the incoming
>pointer happens to be on the same branch. But there's not much you can do to
>detect that, right?

TBH I cribbed this code directly from FastLoadFile's error checking. I think you might be right that this is best-effort.

>Hmm. You could #ifdef ENABLE_TESTS this whole monster?
Turns out you can't, as discussed in chat :(
Did make the interface unscriptable, though.

>Hmm. Why are the NS_NewObjectOutputWrappedStorageStream etc functions inline?
>You could solve this by out-of-lining them in StartupCacheUtils.cpp and
>exporting them, right?
Couldn't get this to work. I'll attach my attempt in another patch after this though, maybe you can see what is wrong.

>Even when it's async, you'll still need some event loop spinnage here: the
>message about the write completing will be posted back to the main thread's
>event queue. Which you're already doing in TestStartupWriteRead, no?
Yeah, this is correct. Actually, the test works, I just had left in an obsolete comment.

Also, made the change that if startupCache still has some entries in memory during shutdown, it won't write them out to disk. I think this is consistent with not wanting to do work during shutdown and also the general best-effort contract of the cache.

I wrote the suggested tests, also.
Attachment #464753 - Attachment is obsolete: true
Attachment #464947 - Flags: review?(dwitte)
Attached patch mega everything patch (obsolete) — Splinter Review
Attachment #465045 - Attachment is obsolete: true
Attached patch mega everything patch (obsolete) — Splinter Review
qparent is changeset 47641

Really need to look into setting up remote patch queue
Attachment #465049 - Attachment is obsolete: true
Attached patch client mozJSComponentLoader (obsolete) — Splinter Review
Replaced references to 'fastloading' with startup cache, and reworked branch logic as suggested. I looked over XPCONNECT_STANDALONE stuff a bit, and I'm pretty sure it doesn't build, but I erred in favor of leaving stuff as it is (maybe for another patch).

>Early returns will drop 'buf' on the floor. You should make 'buf' an
>nsAutoArrayPtr. It'd really be much nicer if you used nsTArray<char> in this
>interface :(
Might be good to look over the GetBuffer/PutBuffer stuff again. I didn't use a nsAutoArrayPtr for the PutBuffer, because PutBuffer always copies now (so we always just delete the buffer). I did switch to AutoPtr for the GetBuffer though. It's unfortunate that you can't pass in a AutoArrayPtr<char>* instead of char**.

Thanks for the help with reviews.
Attachment #442521 - Attachment is obsolete: true
Attachment #465095 - Flags: review?(dwitte)
Also takes out mentions of 'fastload' where they still remained. Might need a quick sr, since I changed the name of an interface function from AbortFastLoads to AbortCaching.

I also removed some code having to do with fastload dependency checking. This shouldn't be alarming, because 1) we were only checking dependencies in DEBUG anyways, and 2) startupcache doesn't have a dependency checking mechanism to begin with. We'll want to add the dependency stuff in (probably independent of fastload) when we get jarred extensions.
Attachment #464599 - Attachment is obsolete: true
Attachments are getting a bit messy, so just to summarize the current state:

Core code has r- from dwitte, new patch up for review. Still needs a minor change with exporting instead of inlining functions. Also needs to be run through valgrind again.

client xulPrototypeCache has r+ from sicking. New patch is up to fix some nits, and I might need quick sr for a method rename away from fastload.

client mozJSComponentLoader has r- from dwitte, but he says it's pretty close. New patch is up for review here as well.

Finally, all this still needs to be run through tryserver and stuff, and possible bustages fixed (some oranges and one build failure last week, so need to investigate that.)
(In reply to comment #89)
> I also removed some code having to do with fastload dependency checking. This
> shouldn't be alarming, because 1) we were only checking dependencies in DEBUG
> anyways,

I don't remember that -- and I don't see any DEBUG ifdefs removed by the patch. What am I missing?

> and 2) startupcache doesn't have a dependency checking mechanism to
> begin with. We'll want to add the dependency stuff in (probably independent of
> fastload) when we get jarred extensions.

Is this going to burn nightly build extension developers in the interim?

/be
I think extension developers will only see different behavior if they are running DEBUG builds. (Nightlies are not DEBUG builds, right?) 

The bit I removed is in nsChromeProtocolHandler, as part of the xulCache patch:
https://bug520309.bugzilla.mozilla.org/attachment.cgi?id=465110

And the ifdef DEBUGs I am referring to are here:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsFastLoadFile.cpp#736

So basically, dependencies are currently only checked in debug builds. Startupcache doesn't support dependency-checking at all, but I'm planning to eventually do some coarser form of compatibility checking during startup instead (have a list of jars we need to stat, then just remove the startupcache file if any of them changed). mwu has some thoughts on jarring extensions to make this possible -- he says he'd like to get it in for FFx4 but is not sure if it's possible.
(In reply to comment #92)
> I think extension developers will only see different behavior if they are
> running DEBUG builds. (Nightlies are not DEBUG builds, right?) 

Almost all extension devs are not running their own builds, let alone debug builds, they are using nightlies or even more likely release builds, both of which aren't debug. We need to care that they have some easy possibility to do rapid development with seeing their changes have effect - in whatever way we enable this. If it doesn't work by default, but needs a pref or so, we need to shout that out really loudly in extension dev channels (i.e. coordinate with the AMO team to get the word out).
The -purgecaches thing we've already been suggesting still works, right?
(In reply to comment #92)
> And the ifdef DEBUGs I am referring to are here:
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsFastLoadFile.cpp#736

That was changed by you only recently, for bug 511761 -- so yeah, nothing in the patch for this bug (bug 520309) makes things worse, but we're still dancing around the developer pain issue of bug 511761.

When a cache relied on by developers that was reliably self-invalidating for years (a decade?) becomes invalid without manual intervention, that is still a problem. But this is for bug 511761.

> So basically, dependencies are currently only checked in debug builds.
> Startupcache doesn't support dependency-checking at all, but I'm planning to
> eventually do some coarser form of compatibility checking during startup
> instead (have a list of jars we need to stat, then just remove the startupcache
> file if any of them changed). mwu has some thoughts on jarring extensions to
> make this possible -- he says he'd like to get it in for FFx4 but is not sure
> if it's possible.

It sounded like this was going to happen. If it isn't, we should not just shrug. We need to have a plan that works for everyone, developers included. Right now the discourse with devs is delayed or broken, mediated mainly by after-the-fact flak-catching.

Asking about whether some manual switch that someone has been "suggesting" does not cut it. Tending the platform means knowing about developer habits, workflow, and usability problems. Unless developers know when to use -purgecaches, they'll have the frustrating experience described by comments in bug 511761.

Why am I going on in this bug? Because I do not think it's good form to bootstrap arguments for manual cache validation based on the recent precedent when that precedent is considered a bug.

And what's more, your last paragraph suggests that we do not know what is "possible", never mind desirable, for the next major Firefox release. We need to know. It's up to us.

/be
Attached patch megapatch (obsolete) — Splinter Review
Attachment #465050 - Attachment is obsolete: true
Comment on attachment 465463 [details] [diff] [review]
megapatch

>diff --git a/chrome/src/nsChromeProtocolHandler.cpp b/chrome/src/nsChromeProtocolHandler.cpp

>@@ -240,54 +240,19 @@ nsChromeProtocolHandler::NewChannel(nsIU

>+    // XXX Removed dependency-tracking code from here, because we're not
>+    // tracking them anyways (with fastload we checked only in DEBUG
>+    // and with startupcache not at all), but this is where we would start
>+    // if we need to re-add.

File a bug and quote # here.

>diff --git a/content/xul/content/src/nsXULElement.cpp b/content/xul/content/src/nsXULElement.cpp

> nsresult
> nsXULPrototypeScript::DeserializeOutOfLine(nsIObjectInputStream* aInput,
>                                            nsIScriptGlobalObject* aGlobal)
> {

>+     nsCOMPtr<nsIObjectInputStream> objectInput = aInput;
>+     if (cache) {
>         PRBool useXULCache = PR_TRUE;
>         if (mSrcURI) {
>             // NB: we must check the XUL script cache early, to avoid
>-            // multiple deserialization attempts for a given script, which
>-            // would exhaust the multiplexed stream containing the singly
>-            // serialized script.  Note that nsXULDocument::LoadScript
>+            // multiple deserialization attempts for a given script.            // would exhaust the multiplexed stream containing the singly

Botched comment.

>diff --git a/content/xul/document/src/nsXULPrototypeCache.cpp b/content/xul/document/src/nsXULPrototypeCache.cpp

> nsXULPrototypeCache::~nsXULPrototypeCache()
> {
>     FlushScripts();
>-
>-    NS_IF_RELEASE(gFastLoadService); // don't need ReleaseService nowadays!
>-    NS_IF_RELEASE(gFastLoadFile);
>+    //NS_IF_RELEASE(gStartupCache); // don't need ReleaseService nowadays!

Remove.

> nsXULPrototypeDocument*
> nsXULPrototypeCache::GetPrototype(nsIURI* aURI)
> {
>     nsXULPrototypeDocument* protoDoc = mPrototypeTable.GetWeak(aURI);
>-
>+#ifdef MOZ_ENABLE_LIBXUL
>     if (!protoDoc) {
>-        // No prototype in XUL memory cache. Spin up FastLoad Service and
>-        // look in FastLoad file.
>-        nsresult rv = StartFastLoad(aURI);
>-        if (NS_SUCCEEDED(rv)) {
>-            nsCOMPtr<nsIObjectInputStream> objectInput;
>-            gFastLoadService->GetInputStream(getter_AddRefs(objectInput));
>-
>-            rv = StartFastLoadingURI(aURI, nsIFastLoadService::NS_FASTLOAD_READ);
>+        nsresult rv = BeginCaching(aURI);
>+        // No prototype in XUL memory cache. Spin up the cache Service.
>+         if (NS_SUCCEEDED(rv)) {
>+            
>+            nsCOMPtr<nsIObjectInputStream> ois;

Misindented.

>+nsresult
>+nsXULPrototypeCache::GetInputStream(nsIURI* uri, nsIObjectInputStream** stream) 
>+{
>+    nsCAutoString spec;
>+    uri->GetPath(spec);
>+    
>+    char* bufPtr = nsnull;
>+    PRUint32 len;
>+    nsCOMPtr<nsIObjectInputStream> ois;
>+    if (!gStartupCache)
>+        return NS_ERROR_NOT_AVAILABLE;
>+    
>+    nsresult rv = gStartupCache->GetBuffer(spec.get(), &bufPtr, &len);

getter_Transfers (see later for how)

>+nsresult
>+nsXULPrototypeCache::FinishOutputStream(nsIURI* uri) 
>+{
>+    nsresult rv;
>+    if (!gStartupCache)
>+        return NS_ERROR_UNEXPECTED;
>+    
>+    nsCOMPtr<nsIStorageStream> storageStream;
>+    PRBool found = mOutputStreamTable.Get(uri, getter_AddRefs(storageStream));
>+    if (!found)
>+        return NS_ERROR_UNEXPECTED;
>+    nsCOMPtr<nsIOutputStream> outputStream
>+        = do_QueryInterface(storageStream);
>+    outputStream->Close();
>+    
>+    char* bufPtr = nsnull;
>+    PRUint32 len;
>+    rv = NS_NewBufferFromStorageStream(storageStream, &bufPtr, &len);

getter_Transfers (see later for how)

>+nsresult
>+nsXULPrototypeCache::HasData(nsIURI* uri, PRBool* exists)
>+{
>+    if (mOutputStreamTable.Get(uri, nsnull)) {
>+        *exists = PR_TRUE;
>+        return NS_OK;
>+    }
>+    nsCAutoString spec;
>+    uri->GetPath(spec);
>+    char* bufPtr = nsnull;
>+    PRUint32 len;
>+    nsresult rv;
>+    if (gStartupCache)
>+        rv = gStartupCache->GetBuffer(spec.get(), &bufPtr, &len);

getter_Transfers (see later for how)

>+    else {
>+        // We don't have everything we need to call BeginCaching and set up
>+        // gStartupCache right now, but we just need to check the cache for 
>+        // this URI.
>+        StartupCache* sc = StartupCache::GetSingleton();
>+        rv = sc->GetBuffer(spec.get(), &bufPtr, &len);

(and here)

>-nsXULPrototypeCache::StartFastLoad(nsIURI* aURI)
>+nsXULPrototypeCache::BeginCaching(nsIURI* aURI)
> {

>+    if (NS_SUCCEEDED(rv)) {
>+        buf.forget();
>+        rv = objectInput->ReadCString(fileLocale);
>+        rv |= objectInput->ReadCString(fileChromePath);
>+        if (NS_FAILED(rv) ||
>+            (!fileChromePath.Equals(chromePath) ||
>+             !fileLocale.Equals(locale))) {
>+            // Our cache won't be valid in this case, we'll need to rewrite.
>+            // XXX This blows away work that other consumers (like
>+            // mozJSComponentLoader) have done, need more fine-grained control).

Superfluous paren.

Raises a good point though. Did fastload separate things by consumer? As in, if XULPrototypeCache and JSComponentLoader both stored something with the same key, did they stomp on each other?

It might be nice to have a consumer string that we tack onto the URI to get the key, e.g. "js:chrome://..." or "xp:chrome://...". That way there's no danger of consumers stomping on each other if they both want to store data for the same URI.

Ask bsmedberg what he thinks.

>diff --git a/js/src/xpconnect/loader/mozJSComponentLoader.cpp b/js/src/xpconnect/loader/mozJSComponentLoader.cpp

>-nsresult
>-mozJSComponentLoader::ReadScript(nsIFastLoadService *flSvc,
>-                                 const char *nativePath, nsIURI *uri,
>+mozJSComponentLoader::ReadScript(StartupCache* cache,nsIURI *uri,

Missing space

>+    nsCAutoString spec;
>+    rv = uri->GetSpec(spec);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    char* bufPtr;
>+    PRUint32 len;
>+    rv = cache->GetBuffer(spec.get(), &bufPtr, &len);

nsAutoArrayPtr<char> buf;
GetBuffer(getter_Transfers(buf));

>+mozJSComponentLoader::WriteScript(StartupCache* cache, JSScript *script,
>+                                  nsIFile *component, nsIURI *uri, JSContext *cx)
> {

>+    char* buf;
>+    PRUint32 len;
>+    rv = NS_NewBufferFromStorageStream(storageStream, &buf, &len);

getter_Transfers here too

>@@ -1258,70 +1013,42 @@ mozJSComponentLoader::GlobalForLocation(

>-    if (NS_SUCCEEDED(rv)) {
>-        rv = StartFastLoad(flSvc);
>+    if (cache) {
>+        rv = ReadScript(cache, aURI, cx, &script);
>         if (NS_SUCCEEDED(rv)) {
>-            fastLoading = PR_TRUE;
>+            LOG(("Successfully loaded %s from startupcache\n", nativePath.get()));
>+        } else {
>+            // This is ok, it just means the script is not yet in the
>+            // cache. Could mean that the cache was corrupted and got removed,
>+            // but either way we're going to write this out.
>+            writeToCache = PR_TRUE;
>+            rv = NS_OK;

We don't use rv after these branches, right? So no need to set it.

>diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp

>+nsresult
>+StartupCache::Init() 
>+{

>+  mObserverService = do_GetService("@mozilla.org/observer-service;1");
>+  
>+  if (!mObserverService) {
>+    NS_WARNING("Could not get observerService.");
>+    return NS_ERROR_UNEXPECTED;
>+  }

  mObserverService = do_GetService("@mozilla.org/observer-service;1", &rv);
  NS_ENSURE_SUCCESS(rv, rv);

>+  
>+  mListener = new StartupCacheListener();  
>+  rv = mObserverService->AddObserver(mListener, NS_XPCOM_SHUTDOWN_OBSERVER_ID,
>+                                     PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  rv = LoadArchive();
>+  
>+  // Sometimes we don't have a cache yet, that's ok.
>+  // If it's corrupted, just remove it and start over.
>+  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) {
>+    NS_WARNING("Failed to load startupcache file correctly, removing!");
>+    InvalidateCache();
>+    rv = NS_OK;

No need.

>+// NOTE: this will not find a new entry until it has been written to disk!
>+nsresult
>+StartupCache::GetBuffer(const char* id, char** outbuf, PRUint32* length)

Add comment about consumer taking ownership of buffer.

>+{
>+  PRBool exists;
>+
>+  if (!mStartupWriteInitiated) {
>+    CacheEntry* entry; 
>+    nsDependentCString idStr(id);
>+    mTable.Get(idStr, &entry);
>+    if (entry) {
>+      char* buf = new char[entry->size];
>+      memcpy(buf, entry->data, entry->size);
>+      *outbuf = buf;
>+      *length = entry->size;
>+      return NS_OK;

Factor this new/memcpy code...

>+    }
>+  }
>+
>+  if (mArchive) {
>+    nsZipItem* zipItem = mArchive->GetItem(id);
>+    if (zipItem) {
>+      PRUint8* itemData = mArchive->GetData(zipItem);
>+      if (!itemData || !mArchive->CheckCRC(zipItem, itemData)) {
>+        NS_WARNING("StartupCache file corrupted!");
>+        InvalidateCache();
>+        return NS_ERROR_FILE_CORRUPTED;
>+      }
>+
>+      PRUint32 size = zipItem->Size();
>+      char* buf = new char[size];
>+      memcpy(buf, itemData, size);
>+      *outbuf = buf;
>+      *length = size;
>+      return NS_OK;

... and this, into a single block. (Declare data & len before these blocks and, after, check for nonnull and new/memcpy.)

>+nsresult
>+StartupCache::PutBuffer(const char* id, const char* inbuf, PRUint32 len) 
>+{
>+  nsresult rv;
>+
>+  if (StartupCache::gShutdownInitiated) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  nsAutoArrayPtr<char> data(new char[len]);
>+  memcpy(data, inbuf, len);
>+
>+  nsDependentCString idStr(id);
>+  if (!mStartupWriteInitiated) {
>+    // Cache it for now, we'll write all together later.
>+    CacheEntry* entry; 
>+    mTable.Get(idStr, &entry);
>+    NS_ASSERTION(entry == nsnull, "Existing entry in StartupCache.");
>+
>+    if (mArchive) {
>+      nsZipItem* zipItem = mArchive->GetItem(id);
>+      NS_ASSERTION(zipItem == nsnull, "Existing entry in disk StartupCache.");
>+    }

These two blocks of code are only for DEBUG, right? #ifdef them as such.

>+
>+    entry = new CacheEntry(data.forget(), len);
>+    mTable.Put(idStr, entry);
>+    return NS_OK;
>+  }
>+  
>+  rv = mZipW->Open(mFile, PR_RDWR | PR_CREATE_FILE);
>+  NS_ENSURE_SUCCESS(rv, rv);  
>+
>+  // XXX We need to think about whether to write this out every time,
>+  // or somehow detect a good time to write.  We need to finish writing
>+  // before shutdown though, and writing also requires a reload of the
>+  // reader's archive, which probably can't handle having the underlying
>+  // file change underneath it. Potentially could reload on the next
>+  // read request, if this is a problem.

File a bug and quote #.

>+#ifdef DEBUG
>+  PRBool hasEntry;
>+  rv = mZipW->HasEntry(idStr, &hasEntry);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(hasEntry == PR_FALSE, "Existing entry in disk StartupCache.");
>+#endif
>+
>+  nsCOMPtr<nsIStringInputStream> stream
>+    = do_CreateInstance("@mozilla.org/io/string-input-stream;1",
>+                        &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = stream->AdoptData(data.forget(), len);
>+  NS_ENSURE_SUCCESS(rv, rv);

I think you want to .forget() after the NS_ENSURE_SUCCESS.

>+PLDHashOperator
>+CacheCloseHelper(const nsACString& key, nsAutoPtr<CacheEntry>& data, 
>+                 void* closure) 
>+{
>+  nsresult rv;
>+ 
>+  CacheWriteHolder* holder = (CacheWriteHolder*) closure;  
>+  nsCOMPtr<nsIStringInputStream> stream = holder->stream;
>+  nsCOMPtr<nsIZipWriter> writer = holder->writer;

Regular pointers here please.

>+
>+  stream->ShareData(data->data, data->size);
>+
>+#ifdef DEBUG
>+  PRBool hasEntry;
>+  rv = writer->HasEntry(key, &hasEntry);
>+  NS_ASSERTION(NS_SUCCEEDED(rv) && hasEntry == PR_FALSE, 
>+               "Existing entry in disk StartupCache.");
>+#endif
>+  rv = writer->AddEntryStream(key, 0, 0, stream, false);
>+  
>+  if (NS_FAILED(rv)) {
>+      NS_WARNING("cache entry deleted but not written to disk.");

2-space indent

>+void
>+StartupCache::WriteToDisk() 
>+{
>+  nsresult rv;
>+  mStartupWriteInitiated = PR_TRUE;
>+
>+  if (gShutdownInitiated) {
>+    NS_WARNING("xpcom-shutdown recieved before initial write, will not write");
>+    mTable.Clear();

The dtor will do this for you, right?

>+    return;
>+  }
>+
>+  if (mTable.Count() == 0)
>+    return;
>+
>+  rv = mZipW->Open(mFile, PR_RDWR | PR_CREATE_FILE);
>+  if (NS_FAILED(rv)) {
>+    // XXX Is there a better / recoverable way to fail here?
>+    NS_WARNING("could not open zipfile for write");

Not really. Just warn and return.

>+    return;
>+  } 
>+
>+  nsCOMPtr<nsIStringInputStream> stream 
>+    = do_CreateInstance("@mozilla.org/io/string-input-stream;1", &rv);
>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("Couldn't create string input stream.");
>+    return;
>+  }
>+
>+  nsAutoPtr<CacheWriteHolder> holder (new CacheWriteHolder());

Declare on stack.

>+void
>+StartupCache::InvalidateCache() 
>+{
>+  mTable.Clear();
>+  mFile->Remove(false);

Do you need to make sure mZipW is closed here? Does it like having its file ripped out from underneath it? Can InvalidateCache be called in circumstances where mZipW is open? If not, assert such.

>+// We don't want to refcount StartupCache, so we'll just
>+// hold a strong ref to this and pass it to observerService instead.
>+// (ObserverService refcounts the objects we pass to it).

ITYM "ObserverService holds a strong reference to", but no need for the last sentence.

>+StartupCacheWrapper* StartupCacheWrapper::GetSingleton() 
>+{
>+  if (gStartupCacheWrapper) {
>+    NS_ADDREF(gStartupCacheWrapper);
>+    return gStartupCacheWrapper;
>+  }    
>+  gStartupCacheWrapper = new StartupCacheWrapper();
>+  NS_ADDREF(gStartupCacheWrapper);
>+  return gStartupCacheWrapper;

  if (!gStartupCacheWrapper)
    gStartupCacheWrapper = new StartupCacheWrapper();

  NS_ADDREF(gStartupCacheWrapper);
  return gStartupCacheWrapper;

>+nsresult
>+StartupCacheWrapper::InvalidateCache() 
>+{
>+  StartupCache* sc = StartupCache::GetSingleton();
>+  sc->InvalidateCache();

Why no !sc check here?

>diff --git a/startupcache/StartupCache.h b/startupcache/StartupCache.h

>+class StartupCache
>+{

>+public:
>+  nsresult GetBuffer(const char* id, char** outbuf, PRUint32* length);
>+  nsresult PutBuffer(const char* id, const char* inbuf, PRUint32 length);
>+  void InvalidateCache();
>+  nsresult GetDebugObjectOutputStream(nsIObjectOutputStream* aStream,
>+                                      nsIObjectOutputStream** outStream);
>+  static StartupCache* GetSingleton();
>+  static void DeleteSingleton();

Comment above these functions like you do in the idl.

>+#ifdef DEBUG
>+  nsTHashtable<nsISupportsHashKey> mWriteObjectMap;

Does this map need to be kept in sync with mTable, or do you want to accumulate everything written over a session? Should you clear this in InvalidateCache()?

>diff --git a/startupcache/test/Makefile.in b/startupcache/test/Makefile.in

>+# Contributor(s):
>+#     Boris Zbarsky <bzbarsky@mit.edu>  (Original author)

Probably not?

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+  file->SetNativeLeafName(NS_LITERAL_CSTRING("startupCache"));
>+  file->Remove(PR_TRUE);

As asked in previous review: Wait, isn't the file 'startupCache.4.big' or something?

r-, want one more look -- just post an interdiff of your changes.
Attachment #465463 - Flags: review-
Attachment #464947 - Flags: review?(dwitte)
Attachment #465095 - Flags: review?(dwitte)
Blocks: 586859
Attached patch new megapatch (obsolete) — Splinter Review
By the way: brenden, I'll discuss with mwu and others and post with a plan. Seems like manual invalidation (purgecaches or otherwise) is unacceptable to you and others, and the old approach is not acceptable to taras, bsmedberg, and others.
Attachment #465463 - Attachment is obsolete: true
(In reply to comment #98)
> Created attachment 465525 [details] [diff] [review]
> new megapatch
> 
> By the way: brenden, I'll discuss with mwu and others and post with a plan.
> Seems like manual invalidation (purgecaches or otherwise) is unacceptable to
> you and others,

Try developers, based on everything the ones commenting in bugs say, and what Jorge says (words like "disaster" have been used).

> and the old approach is not acceptable to taras, bsmedberg, and others.

The old approach of too many stats is not on the table. It's counterproductive to bring it up again. The goal is omnijar-based few-stats, which last I heard was acceptable.

Getting devs to omnijar all the time is a stretch, but could we automate? Mwu has talked about trace-based optimization of a single omnijar that contains all one's current add-ons, which seems like a huge win. If this can be automated...

/be
Comment on attachment 465525 [details] [diff] [review]
new megapatch

This is the same as the previous patch.
a=me to land this for beta4
Attached patch scXULCacheSplinter Review
fixes potential NPE, though I don't think we hit it.
Attachment #465110 - Attachment is obsolete: true
Attached patch startupCache only (obsolete) — Splinter Review
This might fix an error on Windows try, although try is broken for pushes right now.
Attachment #444790 - Attachment is obsolete: true
Attached patch mozJsCompLdr (obsolete) — Splinter Review
Some AutoArray ptr fixes as suggested by dwitte.
Attachment #465095 - Attachment is obsolete: true
Attached patch latest megapatch (obsolete) — Splinter Review
Not sure if dwitte wanted to look this over. Realized that I don't technically have r+ yet on jsCompLdr or base patch.

Current status is, I have two non-random oranges on try right now. One is a Windows assertion when closing ZipWriter (in FileStreams::SetEOF), which might be fixed by closing the archive/reader before closing the writer (will test once I can push to try). The other is test_bug485118.xul | scrollbar has wrong minimum width, which I haven't been able to reproduce and have no real idea how to track down.
Attachment #465525 - Attachment is obsolete: true
Comment on attachment 466166 [details] [diff] [review]
interdiff

>@@ -2925,16 +2925,21 @@ nsXULPrototypeScript::Serialize(nsIObjec
> 
> nsresult
> nsXULPrototypeScript::SerializeOutOfLine(nsIObjectOutputStream* aStream,
>                                          nsIScriptGlobalObject* aGlobal)
> {
>     nsresult rv = NS_ERROR_NOT_IMPLEMENTED;
> 
> #ifdef MOZ_ENABLE_LIBXUL
>+    PRBool isChrome = PR_FALSE;
>+    if (NS_FAILED(mSrcURI->SchemeIs("chrome", &isChrome)) || !isChrome)
>+       // Don't cache scripts that don't come from chrome uris.
>+       return rv;
>+

I recall we discussed this, but would like a bit more background here. Did fastload cache non-chrome? Why is it a bug for you to cache chrome?

> nsXULPrototypeCache::HasData(nsIURI* uri, PRBool* exists)
> {

>     if (gStartupCache)
>-        rv = gStartupCache->GetBuffer(spec.get(), &bufPtr, &len);
>+        rv = gStartupCache->GetBuffer(spec.get(), getter_Transfers(buf), 
>+                                      &len);
>     else {
>         // We don't have everything we need to call BeginCaching and set up
>         // gStartupCache right now, but we just need to check the cache for 
>         // this URI.
>         StartupCache* sc = StartupCache::GetSingleton();
>-        rv = sc->GetBuffer(spec.get(), &bufPtr, &len);
>+        if (!sc) {
>+            *exists = PR_FALSE;
>+            return NS_OK;
>+        }
>+        rv = sc->GetBuffer(spec.get(), getter_Transfers(buf), &len);

How often is HasData() called? If it's anything resembling hot, sounds like we want a StartupCache::HasEntry() API. Stick a printf in here to figure that out, and if warranted file a followup.

>@@ -132,16 +132,24 @@ StartupCache::Init() 
>   mWriteObjectMap.Init();
> #endif
> 
>   mZipW = do_CreateInstance("@mozilla.org/zipwriter;1", &rv);
>   NS_ENSURE_SUCCESS(rv, rv);
>   nsCOMPtr<nsIFile> file;
>   rv = NS_GetSpecialDirectory("ProfLDS",
>                               getter_AddRefs(file));
>+  if (NS_FAILED(rv)) {
>+    rv = NS_GetSpecialDirectory("ProfLD",
>+                                getter_AddRefs(file));
>+  }
>+  if (NS_FAILED(rv)) {
>+    rv = NS_GetSpecialDirectory("ProfDS",
>+                                getter_AddRefs(file));
>+  }

We should remove this stuff now that you're done testing.

Questions remaining from the last review:

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+void
>+StartupCache::InvalidateCache() 
>+{
>+  mTable.Clear();
>+  mFile->Remove(false);

I heard that mZipW.file or somesuch will be null when it's closed. Should you assert that here?

>+  file->SetNativeLeafName(NS_LITERAL_CSTRING("startupCache"));
>+  file->Remove(PR_TRUE);

As asked in previous two reviews: Wait, isn't the file 'startupCache.4.big' or
something?

Finally, let's file a followup for the consumer identifier idea ("js:chrome://..."). Might (likely?) end up as WONTFIX but we should get it on file so we don't forget.

r=dwitte with all that! Once you get tests passing, post another interdiff and I'll take a quick look.
Attachment #466166 - Flags: review+
Attached patch interdiff (obsolete) — Splinter Review
>I recall we discussed this, but would like a bit more background here. Did
>fastload cache non-chrome? Why is it a bug for you to cache chrome?
Yeah, seemed like it could be induced to do so (accidentally, by mochitests, at least). Seems like caching non-chrome is not desired functionality, and I remember asking a few people about this (who agreed).

>How often is HasData() called? If it's anything resembling hot, sounds like we
>want a StartupCache::HasEntry() API. Stick a printf in here to figure that out,
>and if warranted file a followup.
Filed Bug 588334.

>We should remove this stuff now that you're done testing.
Done.

>I heard that mZipW.file or somesuch will be null when it's closed. Should you
>assert that here?
I actually null it out here now, in case we got into an inconsistent state earlier (unexpected failure in PutBuffer or similar).

>As asked in previous two reviews: Wait, isn't the file 'startupCache.4.big' or
>something?
The file resides inside a startupcache/ dir, which lives in profile directory. This theoretically means a user could have multiple startupcaches if he moved platforms, and also means we don't need to pass the annoying full name up here.

>Finally, let's file a followup for the consumer identifier idea
>("js:chrome://..."). Might (likely?) end up as WONTFIX but we should get it on
>file so we don't forget.
Filed Bug 588335.


Re: tests, it looks like after I did a submission with a quick hack of the writeobject assertion stuff, all of the try failures were also present in the base revision. So I've submitted a cleaned-up version, and I expect it to pass. Exciting!
Attachment #466166 - Attachment is obsolete: true
Comment on attachment 466968 [details] [diff] [review]
interdiff

> void
> StartupCache::InvalidateCache() 
> {
>   mTable.Clear();
>+  mArchive = NULL;
>+
>+  // This is usually closed, but it's possible to get into
>+  // an inconsistent state.
>+  mZipW->Close();

Hmm, how does one get into said state? I thought that was only possible through programmatic error.

> NS_EXPORT nsresult
> NS_NewObjectOutputWrappedStorageStream(nsIObjectOutputStream **wrapperStream,
>-                                       nsIStorageStream** stream)
>+                                       nsIStorageStream** stream,
>+				       PRBool wantDebugStream)

No tabs please. Fix your editor. :)

>diff -U 8 -rpN -x . mozilla-central2/content/xul/document/src/nsXULPrototypeCache.cpp mozilla-central/content/xul/document/src/nsXULPrototypeCache.cpp

>         rv = NS_NewObjectOutputWrappedStorageStream(getter_AddRefs(objectOutput), 
>-                                                    getter_AddRefs(storageStream));
>+                                                    getter_AddRefs(storageStream),
>+                                                    false);

A comment here about why you pass false?

>@@ -696,17 +697,18 @@ nsXULPrototypeCache::BeginCaching(nsIURI

>         rv = NS_NewObjectOutputWrappedStorageStream(getter_AddRefs(objectOutput),
>-                                                    getter_AddRefs(storageStream));
>+                                                    getter_AddRefs(storageStream),
>+                                                    false);

And here.

>diff -U 8 -rpN -x . mozilla-central2/content/xul/document/src/nsXULPrototypeDocument.cpp mozilla-central/content/xul/document/src/nsXULPrototypeDocument.cpp

>+#ifdef DEBUG
>+    // XXX Worrisome if we're caching things without system principal.
>+    if (nsContentUtils::IsSystemPrincipal(mNodeInfoManager->DocumentPrincipal())) {
>+        NS_WARNING("Serializing document without system principal");
>+    }

Don't you mean !IsSystemPrincipal()? I assume you don't actually hit this anywhere in tests, though since you appear to have the logic backward I'm guessing you do, in which case you haven't checked? ;)

If this is a legitimate case, we shouldn't warn; if it's really bad but should never ever happen, we should assert; if it can happen we should return failure. Which one is the case?

r=dwitte with satisfactory reply ;)
Attachment #466968 - Flags: review+
Blocks: 523429, 588334, 588335
client mozJSComponentLoader and core stuff is safe to go, so I think it should land first. I'll post an interdiff also.
Attachment #465924 - Attachment is obsolete: true
Attachment #466968 - Attachment is obsolete: true
Attached patch interdiff (obsolete) — Splinter Review
>Hmm, how does one get into said state? I thought that was only possible through
>programmatic error.
For example, if a NS_ENSURE_SUCCESS fails in PutBuffer, this could happen.

I pushed the other things off to the scXULCache patch, because they don't affect this client.
Attachment #468517 - Flags: review+
Attached patch startupCacheCore (obsolete) — Splinter Review
Separating out patches for checkin.
Attachment #464947 - Attachment is obsolete: true
Attachment #465917 - Attachment is obsolete: true
Attachment #468516 - Attachment is obsolete: true
Attached patch mozJsCompLdrSplinter Review
client mozJsCompLdr, for check-in.
Attachment #465918 - Attachment is obsolete: true
These are the remaining failures on tryserver (with nsXULPrototypeCache patch attached).

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | no error while closing the WebConsole - Got true, expected false

ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | scrollbar has wrong minimum width - got 0, expected 22
ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | Small scrollbar has wrong minimum width - got 0, expected 19
ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | scrollbar has wrong minimum height - got 0, expected 22
ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | Small scrollbar has wrong minimum height - got 0, expected 19
One possible reason for the Ts regression is that startupcache writes most entries out on a timer post-startup, so the Talos runs might shut down before that happens.

I see this warning when running Talos locally:
WARNING: Shutting down with entries in startupcache, these will be lost: file /Users/bhsieh/mozilla-central/startupcache/StartupCache.cpp, line 120,
which would be in line with this.

Not sure what the fix might be, yet. Special-casing startupcache to work differently in Talos seems like it would be missing the point.
We could take the shutdown hit and write the startup cache on shutdown if its dirty. That would probably be performance-neutral today. It's a bit hacky as its working around the test behavior, but it's not evil.
How long does it take for it to write everything out? We could have Talos wait longer before closing the browser on the first Ts run. We through away the first run anyway for exactly things like this.

I'm a-ok with modifying the test, since it doesn't actually model anything near real-world conditions. A modification such as the above doesn't violate the intent of the test at all, IMO.
writes on shutdown to fix Ts issues (Ts would shut down before the cache was written).
Attachment #468517 - Attachment is obsolete: true
Attachment #468526 - Attachment is obsolete: true
Attachment #470004 - Attachment is obsolete: true
Attachment #470013 - Flags: review+
Blocks: 591471
Added comment about shutdown write.
Attachment #470003 - Attachment is obsolete: true
Let's mark this FIXED. Benedict, can you file a new bug for the XUL prototype consumer.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 592136
Is there anything special I should do when changing a js component to invalidate this cache? I did changes to nsPlacesAutocomplete.js, recompiled all places and toolkit/library and they were not visible (was enough before), created a new profile and they were visible. Could be this cache related?
I must note this is not a clobber though, so could be clobbering would fix my issue?
(In reply to comment #129)
> Could be this cache related?

Tried manually deleting startupCache folder, and that updated the component code to the latest version, so it's definately related.
Marco, are you on OSX? May be bug 592323, which I hope to fix this week. Doing make should cause a cache invalidation.
(In reply to comment #131)
> Marco, are you on OSX? May be bug 592323, which I hope to fix this week. Doing
> make should cause a cache invalidation.

No, I saw this on Win7, with win7 sdk, latest mozilla-build and pymake.
Marco, I've filed Bug 592944 and cc'd you. In the meantime, you can use the -purgecaches commandline flag to get rid of the caches on each run. Sorry about this.

>Let's mark this FIXED. Benedict, can you file a new bug for the XUL prototype
>consumer.
Filed bug 592943.
Depends on: 593533
Depends on: 598319
Depends on: 597715
No longer depends on: 598319
Blocks: 600713
Blocks: 675191
Depends on: 609710
You need to log in before you can comment on or make changes to this bug.