Closed Bug 68045 Opened 24 years ago Closed 23 years ago

precompile chrome JS and load it incrementally ("fastload")

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: brendan, Assigned: brendan)

References

Details

(Keywords: memory-footprint, perf, topperf, Whiteboard: [nav+perf])

Attachments

(30 files)

step 1: fix JS XDR bugs
44.94 KB, patch
Details | Diff | Splinter Review
diff -wu version of last attachment
11.59 KB, patch
Details | Diff | Splinter Review
a few more nits picked and XXXbe's removed
45.20 KB, patch
Details | Diff | Splinter Review
diff -wu version of last attachment
14.28 KB, patch
Details | Diff | Splinter Review
best patch yet
47.96 KB, patch
Details | Diff | Splinter Review
diff -wu for you curious reviewers
17.67 KB, patch
Details | Diff | Splinter Review
diff -wu of trunk joined with branch (with fixes to undo earlier bad merging in nsHashtable.*)
226.75 KB, patch
Details | Diff | Splinter Review
diff -uwN, includes new files, plus a dbaron fix to use NS_REINTERPRET_CAST instead of NS_STATIC_CAST in nsFastLoadPtr.h for a ** pointer
343.96 KB, patch
Details | Diff | Splinter Review
partial review comments
4.61 KB, text/plain
Details
revisions based on dbaron comments
346.23 KB, patch
Details | Diff | Splinter Review
trunk-based patch to get FastLoad working again
22.68 KB, patch
Details | Diff | Splinter Review
slightly nicer patch, avoids an extra static helper wrapper for the nsSystemPrincipal singleton generic factory "constructor"
23.37 KB, patch
Details | Diff | Splinter Review
big patch to get checksumming and dependency tracking going -- don't review, you want the diff -wu version -- but do apply it
156.03 KB, patch
Details | Diff | Splinter Review
diff -wu version of last patch, plesae review this while you test the last one
100.83 KB, patch
Details | Diff | Splinter Review
proposed checkin comments
2.65 KB, patch
Details | Diff | Splinter Review
forgot xpcom/io/nsFastLoadService.h in the patches, d'oh!
749 bytes, patch
Details | Diff | Splinter Review
replacement diffs for xpcom/io/nsFastLoadFile.{cpp,h} and xpcom/io/nsFastLoadService.cpp
45.27 KB, patch
Details | Diff | Splinter Review
all-in-one patch, sorry for the replacement patches
158.78 KB, patch
Details | Diff | Splinter Review
diff -wu version of last all-in-one patch
103.95 KB, patch
Details | Diff | Splinter Review
latest and greatest all-in-one patch, for tonight's trunk (boy do i want to land this!)
165.87 KB, patch
Details | Diff | Splinter Review
diff -wu version of latest patch, please review this one, it's ready to go!
110.87 KB, patch
Details | Diff | Splinter Review
final patch, for application rather than review
172.19 KB, patch
Details | Diff | Splinter Review
diff -wu version of last patch
117.20 KB, patch
Details | Diff | Splinter Review
Tomi's fix plus saner style in nsFastLoad*.cpp ('if (NS_FAILED(rv)) return rv;' on two lines, so one can breakpoint the return)
195.72 KB, patch
Details | Diff | Splinter Review
diff -wu version of last patch
140.78 KB, patch
Details | Diff | Splinter Review
proposed fix to avoid open-file-remove scenario that works on Unix only
1.58 KB, patch
Details | Diff | Splinter Review
argh, last patch sucked, jrgm rules; try this for better file removal on bad checksum or out of date fastload file
1.64 KB, patch
Details | Diff | Splinter Review
ok, jrgm set me straight once more; I'm ready to trade places with him. Anyway, here's the nsXULDocument.cpp patch you want for close-before-remove
1.69 KB, patch
Details | Diff | Splinter Review
fix so FastLoad file gets recreated after remove, in same session
1.26 KB, patch
Details | Diff | Splinter Review
turn on fastload
821 bytes, patch
waterson
: superreview+
Details | Diff | Splinter Review 
Summary says it all.  Code-level breakdown soon.

/be
Mostly startup perf win, lesser footprint savings (maybe half of 143K for first
browser window?), definitely mozilla0.9 timeframe.

/be
Severity: normal → major
Status: NEW → ASSIGNED
Keywords: footprint, mozilla0.9, perf
Target Milestone: --- → mozilla0.9
Keywords: topperf
One approach:

- Hook into nsComponentManagerImpl::AutoRegisterComponents, alongside
  the xptinfo stuff (and into the installer/regxpcom)
  = walk through the dist/bin/chrome directories and jars, looking for JS files
  = compile them, if there's no pre-compiled file as new or newer, in the
    directory hierarchy, storing chrome://messenger/content/commandglue.js's
    compiled form in dist/bin/chrome/messenger/content/messenger/commandglue.jsc

- Hook into the chrome: loader, to get it to load the .jsc version if it's
  present, when looking for a *.js chrome: URL.

This will cause scripts to not benefit from the JAR-aggregation process, though,
which might lead to a whole new kind of performance pain.  I guess we'll have to
see.

We could also teach the make-chrome process to compile the JS files before
packaging them, but brendan is rightly concerned about having to freeze the
bytecode format, and prefers to have the scripts compiled on the user's machine.

Outstanding issues:
- Is it OK to store the time-stamp data in the registry?  If not, where?
- What do we do with themes and skins that are installed in ~/.mozilla?
- Where should this enumerate-and-compile code live? xpconnect? js/precompile?
  The chrome: code?
- Do we do the same for JS components?  (Why not?)
Assignee: brendan → shaver
Status: ASSIGNED → NEW
is the work for this on track?
I'm taking this one back.  Update soon, it's my main focus for this week
(besides helping get JS1.5 SpiderMonkey RC3 out).

/be
Assignee: shaver → brendan
Keywords: mozilla0.9 → mozilla0.8.1
Target Milestone: mozilla0.9 → mozilla0.8.1

    
    

    
  
Several problems:

- The srcnote vector's terminating SRC_NULL note was not being XDR'ed.

- JSVAL_NULL and JSVAL_VOID were not handled correctly by js_XDRValue.  null
would crash under it, in js_XDRObject; void would be confused for a very
negative int.

Rather than keep compatibility with such bugginess, I just fixed these bugs.  I
don't think anyone cares, since the only use of js_XDRValue in the engine is for
atom keys, and null and undefined are not atomized.  But if some 3rd party used
jsxdrapi.h, they still couldn't depend on the old behavior for null (cuz of the
crash); and for void, they would be depending on a bug where void becomes an int
on decode.

- The JSClass registry in each JSXDRState was written assuming few classes per
XDR stream would have their objects serialized/deserialized.  It now uses a
double hash table if 10 or more classes are registered and JS_FindClassIdByName
is called.  (Hey, why isn't that function named JS_XDRFind...?)

The only cleanup apart from tab elimination and some line-formatting nitpicking
is that I renamed js_XDRNewBase to js_XDRInitBase, because it's a "struct init"
function, not a "new" (instance-allocating) function.  I'd like to move the cx
arg to the end of js_XDRInitBase's parameter list, too. Shaver, any idea whether
someone is using this function?

How about r= from a JS dude and sr= from shaver?

/be
Status: NEW → ASSIGNED

    
    

    
  
I've known about the JSVAL_NULL and _VOID problems for quite some time, but
since that's a Can't Happen case for script serialization, I never bothered to
fix them.

I seriously doubt that anyone is using js_XDR*.  API-frob away!

(I think you named JS_FindClassIdByName, actually. =) )

Good fixes all around; I offer my sr= through a fog of shame.


    
    

    
  
r=rogerl.  This also fixes a long open bug, #31003.


    
    

    
  
sr=hyatt


    
    

    
  
- Get rid of XXXbe before MEM_NEED macro by calling MEM_LEFT from its body's
else clause.

- Avoid single-use locals such as cx in mem_finalize.

- Reorder args to JS_XDRInitBase so as to avoid giving cx priority, which
usually indicates a fallible, error-reporting API entry point but does not here.

- JS_XDRUint8 and JS_XDRUint16 were needlessly masking as well as casting down
to uint8 and uint16, respectively.

- js_XDRString was failing to store zeroes when padding to an aligned boundary.

- Avoid noisy /* !IS_BIG_ENDIAN */ comments after #else and #endif that are only
a few lines away from their controlling #if.

/be

    
  
Blocks: 31003

    
    

    
  
Thanks for all the reviews -- there was a bug in the JS_XDRString address
arithmetic that cleared the padding bytes (memset(raw + nbytes - padlen, ...)
should have been memset((char *)raw + nbytes - padlen, ...), as raw is jschar*).
I'll put patches in the bug and then check 'em in.

/be

    
    

    
  

      
      
      
      

        Attached patch
          
          
        best patch yet
        — 
          — Splinter Review
      

    


  

    
    

    
  
Step 1 patch checked in.  Next step: hack XUL brutal sharing code to save
compiled scripts as needed, and load them instead of compiling their sources if
it can.

/be

    
  
Blocks: 7251

    
  
Blocks: 71373

    
  
Blocks: 71781

    
  
No longer blocks: 7251

    
    

    
  
rest of this is for 0.9, correct?
Target Milestone: mozilla0.8.1 → mozilla0.9

    
    

    
  
Right.

/be
Keywords: mozilla0.8.1 → mozilla0.9

    
    

    
  
What about making mozilla jumpstart chrome file(s)? A jumpstart chrome file
should contain the most wanted chrome parts only! The first window will display
faster and that's why people think it's much faster then before!

I'm sure it can be started just under 5 secs on an average computer system.

    
  
No longer blocks: 71373

    
    

    
  
Not going to make 0.9, and not worth rushing.

/be
Keywords: mozilla0.9 → mozilla0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1

    
    

    
  
The general idea here could speed up startup a fair bit, but it needs more time
to design.  IOW, I don't want to hack just a JS fast-load format, when we should
fast-load all our compiled "code" (XUL, XBL, CSS, JS) from a unified .fasl file.

/be
Keywords: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2

    
    

    
  
What would be the structure of the .fasl file?  I was thinking that it would be
nice for these fast load solutions to be independent of each other (eg when only
the js engine is used in a piece of software), so would a jar holding individual
files be appropriate, whether unified with the existing chrome jars or not?

    
    

    
  
Precompiling the UI on first startup sounds like a nice thing, but it would be a
shame and against the spirit of the web if one day people could just drop in
some compiled stuff in the chrome-directory (or much worse: put it on the web),
so that other folks don't get access to the sources anymore. Imagine the
internet consisting only of Microsoft .chm-files...

    
  
Whiteboard: [nav+perf]

    
    

    
  
See FASTLOAD_20010529_BRANCH for work in progress.  Anyone who figures out why
the changes there (which save a navigator.mfasl file on Unix in one's profile
directory after first startup, which file currently contains serialized scripts,
principals and URL objects; this file is read by subsequent startups to recover
compiled scripts, successfully AFAICT from watching in gdb) seem to cause a XUL
syntax error to be reported (navigator.xul line 318 column 5, unclosed token)
gets a secret reward from me.

Back in five days.

/be

    
  
Target Milestone: mozilla0.9.2 → mozilla0.9.3

    
  
Blocks: 7251

    
    

    
  
Forgot to update this bug the other day: FASTLOAD_20010529_BRANCH works on Linux
now, and builds on Window.  Debugging fastload file deserialization problems on
Windows now.

/be

    
    

    
  
cathleen said her pull from the branch built and ran more than twice on Windows.
Over the phone, I heard her step through nsXULPrototypeScript::Deserialize, so
we think this works on Windows too.  I'm not sure why dbaron's Win2k build did
not work.  I think we're almost there -- any Mac help in sight?

/be

    
    

    
  
waterson tried cathleen's optimized windows build on his sucky 200MHz machine
and measured a 16% speedup.  Woo hoo!

Now I need some advice on hacking CSS code to serialize and deserialize its
expensive-to-compile in-memory data structures.

/be

    
    

    
  
Re: bad PR_Seek behaviour on Mac, with -ve offset.
The code in mac_io.c should do the right thing here (the assertion seems bogus). 
I suspect that if bad things are happening when the assertion is removed, then we 
have other problems.


    
    

    
  
FASTLOAD_20010703_BRANCH is the new branch.  I'm rebuilding it now, hope I
didn't miss a file or merge badly.  Both client.mk and client.mak have been
modified to pull the various modules from the branch.  Anyone who can help me
test this new branch, please have at it.

With some Mac help from peterv (much appreciated), sfraser, and saari, I hope to
land this branch in a few days.

/be

    
    

    
  
is there somewhere a linux or windows binary to download?
I would really like to see this branch :-)

    
    

    
  
My Mac FASTLOAD_20010703_BRANCH build fails with nsIStreamBufferAccess.h not 
found. I guess I need to add an idl file to a project somewhere?


    
    

    
  
I fixed the MANIFEST_IDL files in xpcom/io and xpcom/ds to list all .idl files
that I know are live (nsIBaseStream.idl in io should be cvs removed, I think).

/be

    
    

    
  
.idl files have to be added both to the MANIFIEST_IDL and the IDL.mcp project. 
peterv fixed it.


    
    

    
  
With the hidden window creation commented out (so this works on Mac), I see a ~6% 
improvement in warm start time. (cold start was a tad slower). This is on a fast 
machine (500MHz G4).


    
    

    
  
I have this built on win2k and I'm seeing two glitches. One, when you open some 
dialogs what comes up is a full-screen empty window that is completely white, 
sometimes with a titlebar, sometimes with none at all. Two, probably related, 
some of the 'mfl' files have a length of 32 bytes, which can't be right. 

    
    

    
  
sfraser: cold start can be measured to show a speedup too, if you define cold as
"after reboot, but with a FastLoad file" rather than "without a FastLoad file". 
Waterson's 16% speedup on his 200MHz machine was cold.  Hope I'm not confused;
the point is that cold start should speed up too, once the FastLoad file exists.

jrgm: the hiddenWindow problem sfraser diagnosed is just the tip of the iceberg!

The FastLoad file must evolve to multiplex shared scripts and overlays and help
different, racing applications store (on "cold-meaning-no-FastLoad-file-exists")
and load them coherently.  It'll become more like a dbm file.  I'll be hacking
this up in the 20010703_BRANCH, shooting to land early next week.  Any and all
QA and perf measurement help is most welcome.

/be

    
    

    
  
My numbers were 'cold start with fastload file' vs. 'cold start without fastload 
file', so I think I was doing the correct comparisons. It wasn't significantly 
slower (in the noise of the test, which has a resolution of 1 sec or so).


    
    

    
  
FASTLOAD_20010703_BRANCH has been updated (just now) with fixes and extensions
to handle multiplexing of documents (master and overlay .xul files) within the
FastLoad file and read-or-write process.  Please, please help me test this -- I
need to land the branch very soon.  It works on Linux, and I bet on Windows.  It
should fix the hiddenWindow.xul vs. navigator.xul battle on the Mac.

You should remove any old FastLoad file(s) in your profile directories.  Look
for the new, unified "XUL FastLoad File" (Mac), XUL.mfasl (Unix), and XUL.mfl
(Windows) file, and please suggest a better basename than "XUL".  NB: if there
is an unexpected error reading the file, e.g., due to it lacking a multiplexed
document's scripts, it will be moved aside to "Aborted.mfasl" by DEBUG builds,
and removed by release builds.

/be

    
    

    
  
Ok, try again after updating xpcom/io and xpcom/ds on the branch -- Simon helped
me find a couple of bugs (nsSupportsArray didn't QI to nsICollection; zero net
length or eight byte gross length document segments in the FastLoad multiplex
were not skipped by nsFastLoadFileReader::Read).

/be

    
    

    
  
I'm still seeing problems (udpated mozilla/xpcom to the latest). The fastload 
file is only 320K in size, and I'm getting 'demux underflow' and other assertions 
on starting up with a fastload file.


    
    

    
  
Including the two most recent updates, I have an opt. build on win2k. However, 
:-[ I crash on the second attempt to startup (the one where the .mfl file has
already been created).


    
    

    
  
I blew it, but rev 1.1.4.8 (just checked in) of xpcom/io/nsFastLoadFile.cpp
should make things work better.  jrgm, give it a try -- it doesn't affet the
.mfl file, so as long as the one your build generated is >400KB, you should be
good to go.

sfraser, I don't know why you have a small FastLoad file -- 320KB is clearly way
undersized.  Let's debug tomorrow.  If you can remove it and try again with the
reader fix cited above, please let me know results ASAP.

/be

    
    

    
  
Yep, that works. (My .msf was >400KB as I recall, but for other testing, I'd 
had to blow that profile away, so I started with a new profile). At any rate, 
I went through navigator, composer, mailnews, addrbook and opened every dialog
(xul) that I could find, and didn't notice anything unusual. I'll work on some
timing tomorrow.

    
    

    
  
Sorry Brendan, still no workee. My fastload file is now 504k, but the subsequent 
fastload startup asserts about missing docShellElements, and throws up a big 
blnak window.


    
    

    
  
Whew.  Not only does it transpire that XUL brutal sharing is not enough to share
strres.js (e.g.) when it is included via script src= in distinct .xul files (duh
on that one -- waterson's old XUL script cache makes a comeback, with necessary
changes), but we have to multiplex .js files as we do overlays.  Otherwise, on
Mac at any rate, hiddenWindow.xul and navigator.xul race variously to serialize
shared scripts.

If the race run while writing the FastLoad file differs in order from the race
run while reading, doomage ensued, prior to the latest round of checkins.

Branch buddies, please update, recompile (everything: nsIXULPrototypeCache.h
changed), remove any leftover FastLoad file(s) from your profile directories,
and test again.  Thanks,

/be

    
    

    
  
Sorry, not good. I start OK with no .mfl file, but I have a crash on exit
in that initial run like so: 

JS_STATIC_DLL_CALLBACK(JSDHashOperator)
gc_root_marker(JSDHashTable *table, JSDHashEntryHdr *hdr, uint32 num, 
               void *arg)
{
    JSGCRootHashEntry *rhe = (JSGCRootHashEntry *)hdr;
    jsval *rp = (jsval *)rhe->root;
    jsval v = *rp;                                        //<-- crash

    /* Ignore null object and scalar values. */
    if (!JSVAL_IS_NULL(v) && JSVAL_IS_GCTHING(v)) {
        JSContext *cx = (JSContext *)arg;

 
gc_root_marker(JSDHashTable * 0x00b895e0, JSDHashEntryHdr * 0x022096cc, 
unsigned long 0, void * 0x02549b68) line 891
JS_DHashTableEnumerate(JSDHashTable * 0x00000001, int (JSDHashTable *, 
JSDHashEntryHdr *, unsigned long, void *)* 0x00be74ea 
gc_root_marker(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void 
* 0x02549b68) line 457 + 15 bytes
js_GC(JSContext * 0x02549b68, unsigned int 0) line 1114
js_ForceGC(JSContext * 0x02549b68) line 943 + 12 bytes
js_DestroyContext(JSContext * 0x00000000, int 2) line 228
JS_DestroyContext(JSContext * 0x02549b68) line 882 + 11 bytes
XPCJSContextStack::~XPCJSContextStack(XPCJSContextStack * const 0x00000000) 
line 56
XPCJSContextStack::`scalar deleting destructor'(XPCJSContextStack * const 
0x00000000, unsigned int 1) + 8 bytes
XPCPerThreadData::CleanupAllThreads() line 516 + 13 bytes
NTDLL! 77f89898()

Then, when I go to restart, with the .mfl in place, I just get a big, white
window. [Note: I did clear the .mfl out of the way, and I built from the top
with the most recent changes to the branch].

    
    

    
  
Try again -- I whiffed it in the reader, still (not absolutizing relative script
src= URLs).  However, your crash when writing reeks of bad windows build make
dependencies.  Can you update, clobber, and rebuild for manana?  Thanks,

/be

    
    

    
  
Yeah, I'd begun to suspect that something was wrong with dependencies, and 
I was in the middle of a clobber now. But, I'll pull your other changes,
and restart.

    
    

    
  
Spoke too soon -- one more fix from me tonight, and I'll test write and read all
the way, and report back with a better update/rebuild signal.

/be

    
    

    
  
Testing on Linux here from the branch -- startup seems
if not fast then at least appreciably faster, but better
yet so does 'new window'.  Not bad.  Keep going.  =)

    
    

    
  
Hmm.  Odd.  Sometimes the XUL.mfasl file is created
on startup, sometimes it isn't.  Sometimes it's removed
at shutdown, sometimes it isn't.


    
    

    
  
adam: you're seeing some symptoms of hairy bugs that I've just fixed, combined
with the robustication code that aborts the FastLoad process on write and read,
if things seem to be going south.

I've just fixed some bugs that are too nasty to explain here.  Connoiseurs
should check out bonsai for FASTLOAD_20010703_BRANCH.  Anyway, as Kirk said in
pure ham style in STII:TWOK, "the word is given" -- go test my branch!  It works
for me on Linux (famous last words).

/be

("KKKKKKKKKKKKKHAAAAAAAAAAAAAAAAAAAAAAAANNNNNNNNNNNNNNNNNNN!!!!!!!!!!!!!!!!")

    
    

    
  
Hey Kirk. Are you holding out on a change to nsFastLoadFile.h, like the 
declaration of mSkipOffset?

    
    

    
  
(Too much time with green-skinned chicks and moon princesses, what can I say?)

Fixed, grab 1.1.4.4 of xpcom/io/nsFastLoadFile.h.

/be

    
    

    
  
Rightyho... I've updated mozilla (and xpcom) again
but the XUL.mfasl file is still being randomly removed
at shutdown, and regardless of its existance is ALWAYS
being re-created from scratch at startup, which I assume
is defeating the point.  Linux/x86.

Here is my size (oo-er), for reference:
-rw-r--r--   1 fox        417745 Jul 16 09:13 ./default/XUL.mfasl



    
    

    
  
In my build on win32, the XUL.mfl file is _not_ being removed on exit or 
subsequent restarts. It is created once if it doesn't exist, and is not 
modified after that.

    
    

    
  
Adam, you should probably update in content too.


    
    

    
  
I did some measurements of the change in startup times. The test passes the
startup time in msec into mozilla as part of the command-line URL. The 
end time is when the onload is fired in a trivial HTML file loaded from 
disk. It's not the greatest test in the world, but works as a macro measure
of everything from "double-click" to initial page load. The app can vary 
quite a bit in the time it takes to do a cold start, and with only 3 samples
(because I'm lazy) for the reboot case, I don't think the deltas measured
here are statistically significant (i.e., I don't think this affects startup
time differently for a cold start on the 500MHz machine as opposed to the 
266MHz machine). 

                    # samples  FASL 7/15  Trunk 7/15  Delta (sec.)  % Change
win2k/500MHz/128MB                                                      
  cold boot             3        10.92      10.99         0.07        -0.7%
  subseq.start          9         5.54       6.03         0.48        -8.0%

win98/266MHz/128MB                                                      
  cold boot             3        19.41      20.00         0.60        -3.0%
  subseq.start          9         6.38       6.96         0.58        -8.4%

So, that's on the order of a half-second gain in startup. That understates
the speedup produced by this serialization, since there is a chunk of time
at the beginning (XPCOM init, DLL loading) and at the end (initial page load)
that are independent of this change. 


    
    

    
  
Static build and (based on that) code reorganization to minimize working set are
two very promising avenues being explored in parallel with the FASTLOAD work. 
But I'm hopeful that FASTLOAD will pay off more next week, as hyatt helps me
stuff style sheet data into the mux.

Waterson's stopwatch measurement on his 200MHz machine showed a much greater
(~16%) cold startup win.  I wonder how much RAM that machine had?

/be

    
    

    
  
I've re-updated in content, and xpcom and nspr also, for good
luck, on top of what's picked up by the branch's client.mk.
There was nothing to update since my last comment.  I'm pretty
sure I've got it all.

So my observations stand -- XUL.mfasl is randomly
removed at shutdown, and then rebuilt at startup regardless
of its current [non]existence.

    
    

    
  
Are there some good old-fashioned debugging printf()s
that I can #define-in to help diagnose the problem, if
you don't already have suspicions..?



    
    

    
  
adam: try enabling the NS_BREAK() in nsXULDocument::AbortFastLoads that's
currently #ifdef DEBUG_brendan, and let me know the stack.  If you're building
optimized, stick a printf in the callsites of that routine -- but the stack and
possibly more info available in debug builds would help more (I couldn't tell
whether you were running a debug or release build).

/be

    
    

    
  
This is home-built, but without debug, so I traded
the NS_BREAK() for an abort() and got the following
trace:

[snip]
#3  0x404ce401 in abort () from /lib/libc.so.6
#4  0x40a1589b in nsXULDocument::AbortFastLoads ()
   from /marian/cvs/mozilla/dist/bin/components/libgkcontent.so
#5  0x40a09155 in XULContentSinkImpl::OpenScript ()
   from /marian/cvs/mozilla/dist/bin/components/libgkcontent.so
#6  0x40a086ee in XULContentSinkImpl::OpenTag ()
   from /marian/cvs/mozilla/dist/bin/components/libgkcontent.so
#7  0x40a065fa in XULContentSinkImpl::OpenContainer ()
   from /marian/cvs/mozilla/dist/bin/components/libgkcontent.so
#8  0x407e4b43 in CWellFormedDTD::HandleStartToken ()
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#9  0x407e4820 in CWellFormedDTD::HandleToken ()
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#10 0x407e4393 in CWellFormedDTD::BuildModel ()
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#11 0x407dc6bb in nsParser::BuildModel ()
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#12 0x407dc44d in nsParser::ResumeParse ()
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#13 0x407de429 in nsParser::OnDataAvailable ()
---Type <return> to continue, or q <return> to quit---
   from /marian/cvs/mozilla/dist/bin/components/libhtmlpars.so
#14 0x408150db in nsDocumentOpenInfo::OnDataAvailable ()
   from /marian/cvs/mozilla/dist/bin/components/liburiloader.so
#15 0x4076a572 in nsJARChannel::OnDataAvailable ()
   from /marian/cvs/mozilla/dist/bin/components/libnecko.so
#16 0x4072977b in nsOnDataAvailableEvent::HandleEvent ()
   from /marian/cvs/mozilla/dist/bin/components/libnecko.so
#17 0x40729031 in nsARequestObserverEvent::HandlePLEvent ()
   from /marian/cvs/mozilla/dist/bin/components/libnecko.so
#18 0x4014a3c8 in PL_HandleEvent ()
   from /marian/cvs/mozilla/dist/bin/libxpcom.so
#19 0x4014a2af in PL_ProcessPendingEvents ()
   from /marian/cvs/mozilla/dist/bin/libxpcom.so
#20 0x4014b530 in nsEventQueueImpl::ProcessPendingEvents ()
   from /marian/cvs/mozilla/dist/bin/libxpcom.so
#21 0x4083f6b4 in event_processor_callback ()
   from /marian/cvs/mozilla/dist/bin/components/libwidget_gtk.so
[snip]

I also enabled the Aborted.mfasl move, and FWIW the
Aborted.mfasl file which gets created at startup when
the XUL.mfasl managed to survive from the previous
shutdown is identical to the XUL.mfasl which mozilla
creates in its place.  (Wow, that sentence sucked.)

I'll start the slow process of rebuilding choice cuts of
mozilla with -g for a better trace now, unless you say
'that's enough!'


    
    

    
  
The trace made me sniff out my component.reg, and
sure enough the code was choking at every startup on
(presumably) trying to load obsoleted scripts; nuking
component.reg has eliminated the constant rebuild of
the mfasl file at startup and neither have I seen a
recurrence of the random-deletes-of-file-at-shutdown.

HOORAY!



    
    

    
  
I updated xpcom/io and netwerk/base/src today with some optimizations to stream
buffering for faster i/o (input, actually).

I'd like to land this week, perhaps Wednesday morning?  The CSS-FastLoad work
can go on in the trunk, or in a new branch.

/be

    
    

    
  
Seems to work okay on macos, I get a XUL FastLoad File of 420K. Didn't 
notice much speed improvement on my SCSI G4 500, but this is a debug 
build so all bets are off. It didn't seem worse in anycase.

    
    

    
  
I should note that I had to remove the layout versions of 
nsLayoutAtomList.h and nsXULAtomList.h so my build wouldn't find those 
first and would use the content shared versions instead. I dunno why I'm 
the only one seeing that behavior.

    
    

    
  
saari: can you pull xpcom/io and content/xul/document/src again and rebuild and
retest?  Just for sanity, my changes today are good AFAICT, and they finally
solve a problem jrgm and I have talked about: the my drool-walk-crawl-run theory
of FastLoad devlopment had the FastLoad file contain only whatever was
serialized on the first startup where no FastLoad file existed (e.g., browser
only), and if you started a different set of app components, (e.g. mailnews and
browser), the mailnews XUL document that first loaded and failed to find its URI
mapped in the multiplex would abort the process and remove/move-aside the file.

Just now, I checked in compiling but untested code to handle such hetergenous
startup cases.  Without adding a whole lot more code, the FastLoad read process
continues, but data not deserialized from the file is reparsed/compiled and then
serialized as an update.

jrgm, go nuts!

/be

    
    

    
  
saari: sfraser saw 5-6% startup improvement on his debug build.  We really need
to test optimized to get a good measurement.

/be

    
    

    
  
Do we know how smfr was measuring this? I'm guessing with the Apple profiling
tool...

    
    

    
  
saari: have to ask sfraser when he returns from vacation -- sorry.

all: don't try starting mailnews after the browser fastloads, using a
top-of-branch build -- I need to fix a nasty bug in my grand update code.  More
later tonight.

/be

    
    

    
  
Whew -- see FASTLOAD_20010703_BRANCH bonsai for what I did, cvs comments and
diffs should be intelligible (or I need sleep).  Anyway, things look pretty good
to me.  I ran browser to generate a ~400K .mfasl file, then started mail to get
a 700K total size from the update, then started the mail account manager and
ended up with a 841K file.

I didn't test all the dialogs and prefs to see whether their scripts and event
handlers all worked; any help on that front is much appreciated.  The FastLoad
machinery, including incremental, crash-safe updates, seems to be humming.  Let
me know of any strangeness.  Thanks,

/be

    
    

    
  
brendan: I did a Purify build. Only one problem... UMR because fun_xdrObject 
does not initialize atomstr in the !JSXDR_ENCODE case before using it in the 
call to JS_XDRStringOrNull(xdr, &atomstr). Looks like a trivial fix.

    
    

    
  
With brendan's most recent changes in an opt. build on win2k ... I started 
up mozilla, creating a brand new profile (initial size 470KB). I then 
proceeded to creating a mailnews account and then opening every dialog 
in the application (final size 2,739KB). 

I didn't note any problems (except, maybe, the first time I used directory.xul
[e.g., get a file:// directory listing] the window was blank and did not show 
a file tree. However, on a second and subsequent attempts, things were fine).

I then went through a restart and went back to many of these dialogs with no
problems. I then blew away the XUL file (well, copied it elsewhere) and went 
back through a bunch of dialogs, regenerating the fasl content; no problems.

I had some problems with opening mailnews and browser concurrently (pref 
setting in Prefs->Appearance). I could start composer and mailnews 
concurrently, but when there was a pre-existing .mfl, I couldn't reliably
get navigator to come up (but mailnews always came up). When I tried starting
concurrently mailnews and navigator with _no_ pre-existing .mfl file, on one
try I got navigator, another try mailnews, and a third try I crashed in 
nsXULDocument::AbortFastLoads  with gFastLoadList being null.

However, I could start composer+navigator and composer+mailnews reliably
both with and without an existing .mfl file. [Sidenote: who actually uses
this feature anyways ...]

I also tried manually corrupting the .mfl file, by inserting random characters
in it, or delete lines in the middle. The good news was that it started OK
in browser; the bad news is that it kept the existing .mfl file ... I thought
it would have blown it away.

Anyways, that's all for now. Other things to look at: interaction with 
'-turbo', and checking a full commercial build (AIM,PSM,etc.) although that's 
not at the top of my list right now.



    
    

    
  
Does anything have to happen to existing chrome for this to work?  Also, does it
pick up changes/new installs of chrome properly?  I'm going to try and pull the
branch monday and build commerical with it to see what happens.

    
    

    
  
jband: thanks -- fixed.

jrgm: I haven't yet got the checksumming code implemented, so random corruption
in the middle may not be noticed.  It might just hit some script metadata used
to map line numbers, e.g., and have fairly innocuous effects.

kerz: the FastLoad file format includes make-like dependencies by which the file
can be invalidated if any constituent files look newer, but I have not hooked up
dependency generation yet.  I'll work on that tonight.

I'll look into the browser+mailnews race problems now.  Thanks for all the help,

/be

    
    

    
  
content/xul/document/src/nsXULDocument.{h,cpp} are updated to handle a case that
may have led to the crash jrgm saw (null gFastLoadList, but really: a deleted
nsXULDocument still on the list).

More in a bit.

/be

    
    

    
  
Purple Alert!  If you have rev 3.61.42.1 of js/src/jsfun.c, update it again from
the branch and remove your FastLoad file.  All should be well after that.  Sorry
about the trouble.

/be

    
    

    
  
I just pref-disabled-by-default XUL FastLoad -- you'll need to set
nglayout.debug.disable_xul_fastload to true in your all.js (but don't check in)
or in user.js (I have a change callback set up).  Please enable, all ye testers?

/be

    
    

    
  
D'oh!  I mean set nglayout.debug.disable_xul_fastload to false, to enable XUL
FastLoad.  Darnit,

/be

    
    

    
  
I could use more reviews.  The patch touches caps, netwerk, and mailnews, for
instance.  Please help me land this by Tuesday night.

/be

    
    

    
  
mailnews changes look OK, assuming nothing bad is going to happen because those
methods are stubbed out.

    
    

    
  
Looks generally OK to me. Is there any risk of an attacker being able to use
principal de-serialization to forge a principal?

    
    

    
  
bienvenu: stub nsISerializable methods are ok for now, as no mailnews nsIURI
subtypes are serialized yet.

mstoltz: if the bad guy can get to your FastLoad file, you are doomed.  Same as
if there were no FastLoad file and the attacker modified a chrome .js file.  I
am not reducing or increasing security.

Argh, darin is probably reading through a huge mail backlog.  Cc'ing dougt and
gagan (who's here at the conference) for netwerk review.

/be

    
    

    
  
A one quick comment before i digest your buffered stream changes.  I am not sure
about the nsIURI deriving from a nsISerializable.  I think that it only the URI
implementations that support serializablity should need to implement it.  Thoughts?




    
    

    
  
What is the need for nsIStreamBufferAccess; doesn't ReadSegments work?  (Heck, I
don't even see any callers in this patch);

    
    

    
  
dougt: nsISerializable is on the nsIURI inheritance chain to avoid yet another
vptr per URI or URL instance.  This may not be a significant savings, but in
general if an interface is implemented by a populous object to be serialized
(e.g., nsIContent), I think we do better to chain to nsISupports through
nsISerializable.  The more we move into the FastLoad file, the greater the
number of objects that need to support nsISerializable.  So I saved a vptr per
instance at the cost of three stub methods per URI concrete class.

The nsIStreamBufferAccess interface will be used shortly.  It is not redundant
w.r.t. ReadSegments, because it is meant to be used by nsISerializable::Read and
nsISerializable::Write impls directly.  Consider 1000 content nodes, each with
10 data members to serialize (made-up numbers, but within a binary order). 
Instead of 10000 calls to Read32 or Write32, which call through Read or Write on
the underlying stream, finally reaching a buffer, we do 1000 GetBuffer calls and
1000 PutBuffer calls and 10000 loads or stores (with byte swapping on little
endian platforms).

nsIStreamBufferAccess also supports buffer disable and enable methods, so you
can use direct i/o for small things (like the FastLoad file header) and avoid
dumping a just-filled input buffer.

Finally, I moved the SWAP32, etc. macros from nsBinaryStream.cpp to
nsIStreamBufferAccess.idl's %{C++...%} block, and provided NS_ prefixes and GET
and PUT macros for the primitive types.  These are meant to be used with a char*
returned by nsIStreamBufferAccess::GetBuffer.  Sun XDR has a similar "inline
buffer access" scheme, which NFS uses to save cycles and avoid calling through
multiple layers, exactly as I propose to do with hot FastLoad data structures.

/be

    
    

    
  
1. I am a little concerned about the issues that doug raised too about making
nsIURI be derived from nsISerializable. Since a significant set of nsIURI
implementors (JARURI, SimpleURI, etc) don't have (as yet) a useful reason to
implement nsISerializable. Seems like QI'ng for nsISerializable (and failing in
these cases) would keep it more optimized than calling into no-op functions, no?
Also be aware that JARURI, SimpleURI, etc. have a more important concern for
performance than other URI types. So having a good reason behind making things
have better performance is important.

2. wrt URL's read and write calls, should we worry about version issues and
possible incompatibilities arising due to changes in stream format? (ie. someone
messing up in how they write and read to and from streams) I am fine with the
way it is right now as long as we could ensure that.

3. small nitpick about nsStdURL::GetCID-- is myCID var needed? 


    
    

    
  
Brendan:

This seems like very cool stuff :-)

As far as this business of whether or not nsIURI should inherit from
nsISerializable, I have my doubts.  I say this primarily because being
serializable is not a necko requirement.  Also, nsIURI is a "nearly frozen"
interface, which is implemented by code outside the mozilla codebase.

Making nsISerializable part of the inheritance chain says to me that its methods
must be implemented by all nsIURI implementations, otherwise it is an optional
interface and should be accessed via QueryInterface.  When I look at your patch,
I see that the nsISerializable methods are not implemented by all nsIURI
implementations, so I really think that nsIURI should not inherit from
nsISerializable.

It sounds like there are some URI types that you want to serialize and others
that will never need to be serialized.  If this is indeed the case, then I
really think that QI over inheritance is the answer.

    
    

    
  
gagan: re (1), I do not see a performance penalty to nsIURI inheriting from
nsISerializable, and the stubs are in fact failure-generators, which mean that
if you ever try to serialize one of these NOT_IMPLEMENTED URI subtypes, you'll
get a fatal serialization error that suggests you need to write the actual code
to make these methods succeed.  Serializability is viral, it's like const.  If
eventually, as I expect, nearly all URI subtypes need to be serializable, we
won't have so many stubs.

Re: (2) the FastLoad file is versioned as to metadata format, and the XUL
document code versions its data.  I'm working on the checksum support now.

Re: (3) myCID is needed -- C++ won't let you assign an aggregate initializer to
a lvalue, you can do that only in a declaration.

Darin: sold, I did not know nsIURI was frozen (or nearly so).  If you don't mind
the creeping MI bloat (a word per instance for yet another vptr), I will redo
the code to take nsISerializable out of nsIURI's inheritance chain.

/be

    
    

    
  
cc'ing valeski b/c there may be other interfaces that are frozen or nearly frozen.

    
    

    
  
no other interface melting going on in the patch... just nsIURI. 

I like the idea of URI's being serializable (though I'm not sure how
necessary/practical/useful it is right now. are you seeing some strong gains by
making them serializable?), but I'd prefer QI access rather than MI. Why MI?

    
    

    
  
jud: everything in the XUL prototype document, URIs, scripts, style sheets, and
even decoded images, is likely to want to be serializable.  I used SI (not MI)
at first to avoid yet another vtbl ptr per instance, with QI as usual.  The only
issue was not whether to QI, it was how to inherit nsISerializable.  I've redone
things so that the nsIURI implementations reached by XUL prototype docs inherit
nsISerializable independently from nsIURI.

New patch coming up later tonight, based on current trunk.

/be

    
  
Summary: precompile chrome JS and load it incrementally → precompile chrome JS and load it incrementally ("fastload")

    
    

    
  
Okay, so I built with that latest patch and a trunk pulled at 2am
last night. It's alive....

1) I went through all the xul dialogs in mailnews, addrbook, navigator
   composer and chatzilla (i.e., not including aim, psm, (other?)), and
   periodically shutting down and restarting. Final .mfl size = 3018KB  

2) I didn't note any breakage in any dialog, although I wasn't doing 
   a full functional test for every dialog. But signing up for new 
   news or mail accounts, setting various preferences, replying/forwarding
   mail, adding address card, filing bookmarks, adding sidebars, etc. 
   were all working fine.

3) I had previously (10 days ago) seen an occasional shutdown crash, but 
   never hit it with this build (maybe some fix by dbaron for js gc roots).

4) I could start with any concurrent combination of mailnews|composer|navigator
   (This had not worked reliably before)

So, looking good. To try this out (once this patch hits the trunk) set this in 
prefs.js: 'user_pref("nglayout.debug.disable_xul_fastload", false);'

Still to do: evil file corruption tests, and timing tests.


    
    

    
  
I've set the pref in prefs.js and launched browser + mail + preferences dialogs
but the size of XUL.mfl is only 4K whatever I do

I'm running a home build on Win2k from latest sources and it is a static build

questions to save some time:

Is it supposed to work with a static build ?
Do I need to create a new profile (mine is a bit old) ?
Should I put the pref in all.js also ?

    
    

    
  
Bernard: you need to apply the latest patch in this bug ("trunk-based patch
..."), and it sounds like you have not done that.

/be

    
    

    
  
Hmm.  Bug 92859 has a patch that, when merged up and checked in, may make my
patch to caps/src obsolete.  I'll help with that and work on checksumming, and
perhaps come back with a better patch still before going for review.  If all you
testing buddies can limp along by appling the latest patch here (the "slightly
nicer patch") until then, I'll be ever so grateful.

Sorry this isn't working out of the box, but if vidur's patch in 92859 fixes
things so I don't have to make caps create principals using CreateInstance, I
think it's worth the wait.  If the wait requires too much patch applying by
interested parties, I'll check in the interim solution.

/be

    
    

    
  
Sorry for the spam.

/be
Depends on: 92859
No longer blocks: 7251

    
  
Blocks: 7251

    
  
Target Milestone: mozilla0.9.3 → mozilla0.9.4

    
    

    
  
Revising dependency.  Bug 93792 has a patch that relieves caps/src/ code from
having to CreateInstance its own concrete types.  I'll get that in, then redo a
patch for this bug.

/be
Depends on: 93792
No longer depends on: 92859

    
  
No longer depends on: 93792

    
    

    
  
Adding to CC - sorry for the spam.

    
    

    
  
This bug has mozilla0.9.2 as a keyword. Update or remove?

    
    

    
  
Looking for dbaron r= and waterson/jst/hyatt sr=.

/be

    
    

    
  
I crave shaver's review, as always!  bugmail spam gets him to give it up, usually.

/be

    
    

    
  
Sorry, I missed a crucial step in nsFastLoadFileWriter::Close (storing the fresh
checksum, so it can be retrieved later via nsIFastLoadFileControl::GetChecksum
and cached in the service).  I also took this opportunity to eliminate an XXXbe
comment in nsFastLoadService::StartMuxedDocument, by making the downcast from
nsIObjectInputStream* to nsFastLoadFileReader* in NS_NewFastLoadFileUpdater
type-safe, using ye-olde-QI-to-CID hack.  Bletch!  But it works and is minimal.

/be

    
    

    
  
the netwerk changes seem fine for the most part, but can you explain
nsISeekableOutputStream?  why the change from |fill| to |read|?  why can't
nsIInputStream be used in place of this interface?

    
    

    
  
darin, the short answer is a question: do you really want nsFileOutputStream to
implement all of nsIInputStream?  Before I added nsISeekableOutputStream (which
arguably should be named nsISeekableBufferedOutputStream, or even the unbearable
nsIBackwardsSeekableBufferedOutputStream :-), the code in netwerk/base/src/
nsBufferedStreams.cpp that implements nsBufferedOutputStream::Fill was a no-op,
which is just broken: if you seek back in such a stream and miss the buffer,
then write less than a full buffer's worth of data, then seek again out of that
buffer, or close, you'll write garbage.

Ok, that's recap.  As for why I renamed fill read, see my comment in attachment
45894 [details] [diff] [review]:

- nsISeekableOutputStream inherits from nsISeekableStream, to reduce vtbl and
  QI overhead.  This does require three forwards from nsBufferedOutputStream
  to nsBufferedStream superclass methods.  Also, nsISeekableOutputStream.fill
  was not primitive, in light of seek, so it's now read -- which is exactly
  what nsFastLoadFileWriter checksumming code wants on the unbuffered stream.

Hmm, that was terse.  What I mean by "not primitive" is that fill was
read+seek-back-to-buffer-start-offset, whereas seek was already exposed as a
primitive in the inherited-from-nsISeekableStream nsISeekableOutputStream
interface.  It seems better to me to make interface methods orthogonal and
primitive, unless there is a compelling overhead reduction (common sub-method
elimination) in composing primitives, or perhaps if there is a common
composition that everyone uses frequently (but the latter should be done
generically, not by each interface impl, say via a "NS_ComposeAplusBHelper"
inline or extern function, or perhaps service method, that calls A then B).

So having decomposed nsISeekableOutputStreams's inherited and direct methods
into primitives (setEOF still seems out of place to me, btw :-), I then saw that
I could use read to compute the checksum in the nsFastLoadFileWriter case.  This
was serendipity, or a hack, depending on your taste.  A more elaborate
alternative would be to use nsIStreamIO or nsIFileIO, but (a) FastLoad is in
xpcom, not netwerk; (b) it seemed overkill again, although not as much overkill
as making the stream under a buffered, seekable output stream also implement
nsIInputStream.

/be

    
    

    
  
Here comes a patch that removes nsISeekableOutputStream, which turned out to be
unnecessary once I had fixed nsBufferedOutputStream::mFillPoint to be a high
watermark in the buffer, rather than a copy of mBufferSize (I fixed that while
on the FASTLOAD_20010703_BRANCH, but failed to retest or otherwise realize that
the fix relieved the need for nsISeekableOutputStream; before I changed what
mFillPoint meant, a read-modify-write on the buffer was necessary or garbage
would be written).  Thanks to darin for setting me straight.

The files that changed are

content/xul/document/src/nsXULDocument.cpp
netwerk/base/src/nsBufferedStreams.cpp
netwerk/base/src/nsBufferedStreams.h
netwerk/base/src/nsFileStreams.cpp
netwerk/base/src/nsFileStreams.h
xpcom/io/nsFastLoadFile.cpp
xpcom/io/nsFastLoadFile.h
xpcom/io/nsFastLoadService.cpp
xpcom/io/nsISeekableStream.idl

Still looking for r= and sr= love!

/be

/be

    
    

    
  
I knew I should have waited for my build to finish.  If you apply the last diff
-u patch, please apply this further patch:

+++ ./nsFastLoadFile.cpp	Thu Aug 16 20:44:12 2001
@@ -1705,7 +1705,7 @@

     // XXXbe seek back to eof to force a flush -- why is there no interface for
     //       flushing a buffered output stream?
-    rv = seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);
+    rv = seekable->Seek(nsISeekableStream::NS_SEEK_END, 0);
     if (NS_FAILED(rv)) return rv;

     // Now compute the checksum, using mFileIO to get an input stream on the

There's no way to force a flush on a buffered stream without seeking far from
the current offset.  The - line above worked in the previous patch, where I'd
deoptimized nsBufferedStream::Seek to dump its buffer when seeking below the
cursor, but still within the buffer.  There was no good reason for that
deoptimization once I fixed nsBufferedOutputStream::Flush to use mFillPoint as
the high-watermark when writing the buffer.

Sorry, and I hope this does the trick!

/be

    
    

    
  
r/sr=waterson

    
    

    
  
New patch coming up, I screwed up and failed to fix the mFillPoint
high-watermark maintenance in nsBufferedOutputStream::Write.

/be
assignee_accessible: 1 → 0
CC list accessible: false
qacontact_accessible: 1 → 0
Not accessible to reporter

    
    

    
  
Ok, I've tested twice, cut once this time.  The changes from the last patch are
to these files:

- xpcom/io/nsFastLoadFile.cpp
    Revised to use nsIStreamBufferAccess to flush the header, rather than seek
    to EOF; also write the checksum directly, via the unbuffered output stream.
    This required nsBufferedOutputStream to implement nsIStreamBufferAccess.

- netwerk/base/src/nsBufferedStreams.{h,cpp}
    Fixed mFillPoint maintenance: it was not being updated before each Fill
    call in the Write loop, only after the loop.  But Fill has for the last few
    patches used mFillPoint as the high-watermark in the buffer through which
    to write.  Also, nsBufferedOutputStream implements nsISBA.

- netwerk/base/src/nsStdURL.cpp
    shaver caught a good of mine, assuing IsDirectory to be infallible.

I'm gonna land soon, having got shaver and waterson blessings (shaver via IRC).

/be

    
    

    
  
Tired, but I hope anyone who is following this nonsense knows that I meant
"Flush()", not "Fill" in the blurb for nsBufferedStreams.{h,cpp}'s changes.

Thanks again to darin, dbaron, shaver, waterson, and esp. jrgm, who needs to
name his favorite comestible -- I'm buying.  THe patch just went in.

/be
assignee_accessible: 0 → 1
CC list accessible: true
qacontact_accessible: 0 → 1
Accessible to reporter

    
    

    
  
Fixing database corruption caused by bug 95743.  Pay no attention to the man
behind the curtain.

    
    

    
  
Bother.  Is it just gcc-2.96 optimized builds that crash with this patch?  See
bug 95785, fixed by backing me out.



    
    

    
  
My 2.96 build was optimized without debug but with debugging symbols and I
wasn't able to start the browser.  Also, if I remember correctly HP wouldn't
start up either.

    
    

    
  
No, it was egcs 1.1.2, HP, gcc 2.95, and gcc 2.96 (but not gcc 3.0, Windows, or
Mac).

    
    

    
  
Problem seems to be uninitialized variable (rv) at 
nsChromeProtocolHandler::NewURI()

If i remove line:
if (NS_FAILED(rv)) return rv;

build starts ok.

    
    

    
  
Yep, the do_CreateInstance should have |&rv| as the optional second parameter. 
That would explain this perfectly.

    
    

    
  
*** Bug 95888 has been marked as a duplicate of this bug. ***

    
    

    
  
Please note the comment in bug 95888.

    
    

    
  
Tomi, I could kiss you!  Er, perhaps not -- but let me know your address and
name your favorite fine aged wine, beer, tequila, or whatever beverage you like.

New patch coming up today.

/be

    
    

    
  
brendan:
looks like you are really close. can you make it for 0.9.4?  :-)

    
    

    
  
I've run through about every dialog I could find, with a build on win2k that
had the latest fast load changes. I went through both with fast load enabled
and with it (default) disabled. Both covered navigator and dialogs, composer 
and dialogs, mail and news, msg compose, bookmars, history, js console, 
preferences and subdialogs, and account/profile wizards usage, etc.

I didn't encounter any problems with normal use (disabled) due to the fastload 
changes. 

There were however a couple of things to note: 
1) the fastload enabled profile stopped updating the .mfl file after a while.
   I don't know when this started happening.
2) there were problems with various popups in the trunk build I had, but those
   same problems already existed in a trunk build from the same time that 
   didn't have the fastload changes. 

    
    

    
  
> I didn't encounter any problems with normal use (disabled) due to the 
> fastload changes

I should have said also that I didn't encounter any problems when it was 
enabled either, aside from having it stopped serializing. However, when it
was in that state, the apps all appeared to work fine (although I don't know
whether it was de-serializing from .mfl at that point).

    
    

    
  
So. I am stupid. In the course of looking at the popups issue noted above, 
I happened to run the current trunk with the same profile with the XUL.mfl.
This trunk build will honour that pref, but in a broken way. I think that
is what horked the serialization file, and is a complete false alarm.

As for the popup issues, a clobber cleared them up, in either the current 
trunk, or current trunk plus latest patch.

    
    

    
  
Tested optimized and debug on rh7.1, gcc2.96.  All good, checked in.  Still you
must set the "nglayout.debug.disable_xul_fastload" pref to false to enable, and
if you want to measure performance gains, please also measure with the new pref
"nglayout.debug.checksum_xul_fastload_file" set to false, as well as set to or
left at its true default.

jrgm, the FastLoad file will reach a maximum size for a given set of XUL scripts
loaded by chrome URIs.  Maybe you hit that point?  How big was the file?

/be

    
    

    
  
The XUL.mfl file was 1213KB (1,241,119 bytes) at the point that it stopped 
growing. But I assume that it was my error that caused some undefined state
to be reached, and this is not that hard upper limit.

    
    

    
  
/me should read prior comments before commenting.

jrgm: I suspect running two builds sharing the same profile could lead to a
FastLoad file that's not updated, but I'm not certain.  Let me noodle on that,
and let me know if it happens again.  I don't expect crashes or hangs from such
races, although the whole area of multiple instances sharing a profile is rife
with bugs, I'm sure.

Everyone who can help, please update from the trunk, flip the pref, and test the
heck out of FastLoad.  With enough testimony of only good effects, maybe we can
turn this on for 0.9.4.

/be

    
    

    
  
I'm using 2001082109 , and I'm seeing crashes with Fastload enabled.
For example, when I load MailNews

TB34337815M
TB34337782W

If I disable Fastload, it works fine.

    
    

    
  
WD, we've been here before.  Until I checked in just within the last hour,
FastLoad was not in working order on the trunk.  Either pull and build your own
browser, or wait till tomorrow's builds are available, before crying wolf.

/be

    
    

    
  
For reference, here are some sizes for the serialized file after using
major parts of the app:
  + navigator + bkmk sidebar == 488KB
  + prefs + all prefs panels + all pref subdialogs == 918KB
  + all other dialogs directly reachable from nav (including
    bookmarks, history, form manager, js console, help viewer)
    and XUL sidebars (history/bookmarks/search) == 1151KB
  + all mailnews, including wizards, news/mail subscribe, search, 
    addressbook, msg compose, account properties, etc. == 2338KB
  + chatzilla == 2632KB
  + composer and all dialogs == 2855KB
(does not account for security UI or NS AIM, and may have missed some 
dialogs [almost certainly missing some]. Also, some UI is shared in 
many places, so later counts may underestimate the count they would 
register on their own).

------------------------------
I also tried corrupting the XUL.mfl (by cutting a couple thousand 
characters out of the middle of the file). At that point, instead
of chucking the file, it kept it in place and `touch' the file 
every time I opened a dialog (but added no further bytes to the file).
This is on win2k, and I had not set any pref for 
nglayout.debug.checksum_xul_fastload_file (i.e., checksum was on).

    
    

    
  
Wow.  From what jrgm reports I can only conclude that nsIFile is implemented
differently (is broken) on win2k.  If the checksum doesn't match, the file is
deleted using nsIFile::Remove.  At least, that's what nsXULDocument.cpp plainly
does, and that's what I see happen with my optimized Linux build -- I used emacs
to munge the file good and proper, then ran mozilla and the file disappeared.

Anyone else running the patch or (since the checkin) an updated build, please
sound off with your own results.

The FastLoad file size could be shrunk a bit, as strings shows (it discloses
numerous repeated strings).  The JS engine keeps identifiers uniquely interned,
but when serializing scripts, there is not yet any external atom table.  That
should be done; I'll get on it.

/be

    
    

    
  
Weird... on my linux build, I altered XUL.mfasl (in Vim :) ) and ran it again. 
At first I thought that the file had disappeared, until I noticed that it had
been renamed Invalid.mfasl.

    
    

    
  
Actually, that's the expected behaviour in a debug build (move it aside to that 
filename). In optimized, though, it should nuke it.

    
    

    
  
So this works correctly all the way to the point in nsIFile::Remove() where 
it is going to delete the file with |int remove(const char *path)|. I think
it fails because the file is still open and the docs say the file must be
closed before remove can be called. I'll file a bug.

    
  
Depends on: 96388

    
    

    
  
Just trying fastload with 08/22 nightly on WinNT. I created a fresh profile for 
testing. Mozilla starts fine without fastload, if I turn it on it creates a 
277kb large XUF.mfl and stalls. No window at all appears (after choosing the 
test profile in the Profilemanager window).

    
    

    
  
OK, forget the fubar I posted. Creating a new profile on an *updated* build was
just not working. After *completely* deinstalling and reinstalling Mozilla
"fastload" also works on Win like a charm. Sorry.

    
    

    
  
I think bug 96388 is no longer a dependency: it calls for a doc-comment fix to
nsIFile.idl, but the FastLoad code must take care not to call nsIFile::Remove on
a file to which there are open streams.  Patch to nsXULDocument.cpp that takes
such care coming right up.

/be

    
    

    
  
I've tried re-enabling my pref for fastload and it's not creating the XUL.mfl
file.   This is with 2001082403.   Yesterday's build had the same problem.

    
    

    
  
OK, please disregard my last comments.   I'm an idiot!   :)

    
    

    
  
a=asa for the last patch for fastload testers on mac and windows (not enabled by
defualt)

    
    

    
  
attachment 46840 [details] [diff] [review] is in.  Anyone witha clue on the Mac i/o (seek?) problems that
sometimes cause a runaway FastLoad file, please jump in over at bug 95888.

/be

    
    

    
  
I forgot to mention one nit with the last patch. WIth that  checked in,  a
corrupt file is cleanly removed on windows now (need to check Mac), but when it
goes to create
a new file, it does not set the flags to create it. See
http://lxr.mozilla.org/seamonkey/source/content/xul/document/src/nsXULDocument.cpp#4442
and that mInputStream is non-null (how come?).

All this means is that in the event of a bogus/corrupt file, it will be removed
but fastload
will not be used (not serialized to disk) until the next time the browser is
started. Good enough for now; wants a fix someday.

    
    

    
  
Please excuse this spam...but how does a user set a pref so that it persists
past the next install of a nightly?  Platform is win2k.  I added the pref to
prefs.js (in my moz profile) but that didn't live past today's moz install.  Thanks.

    
    

    
  
<QA Ignore>
Burleigh, did you set this pref with Mozilla shut down?  Hand editing the
prefs.js must be done without mozilla running.  (The file will be read on
startup and rewritten at shutdown.)
</QA Ignore>

    
    

    
  
sr=waterson

    
    

    
  
r=jrgm

    
    

    
  
This won't be on by default in 0.9.4, but it will in 0.9.5 (with XBL added to
FastLoad, for greater speedup).  I do intend to get bug 95888 fixed for 0.9.4,
however, so that those who choose to enable FastLoad get good results.

/be
Keywords: mozilla0.9.2 → mozilla0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

    
  
As per discussion with jrgm, changing QA contact to him -
QA Contact: pschwartau → jrgm

    
    

    
  

      
      
      
      

        Attached patch
          
          
        turn on fastload
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 52847 [details] [diff] [review]
turn on fastload

sr=waterson

        Attachment #52847 -
        Flags: superreview+

    
    

    
  
r=hyatt


    
    

    
  
Turned on.  If you have troubles that seem to be caused by FastLoad, please open
new bugs.

/be
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
removing item for this bug from the release notes since the bug is marked
fixed. If this is in error, please make a note in the release notes bug for
the current milestone. As of today, this is bug 133795. In a ew weeks, it 
will be some other bug.

    
    

    
  
VERIFIED since fastload cached chrome has been present for over a year.
Status: RESOLVED → VERIFIED
Flags: testcase-
Flags: testcase-

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: