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

VERIFIED FIXED in mozilla0.9.5

Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({footprint, perf, topperf})

Trunk
mozilla0.9.5
footprint, perf, topperf
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nav+perf])

Attachments

(30 attachments)

44.94 KB, patch
Details | Diff | Splinter Review
11.59 KB, patch
Details | Diff | Splinter Review
45.20 KB, patch
Details | Diff | Splinter Review
14.28 KB, patch
Details | Diff | Splinter Review
47.96 KB, patch
Details | Diff | Splinter Review
17.67 KB, patch
Details | Diff | Splinter Review
226.75 KB, patch
Details | Diff | Splinter Review
343.96 KB, patch
Details | Diff | Splinter Review
4.61 KB, text/plain
Details
346.23 KB, patch
Details | Diff | Splinter Review
22.68 KB, patch
Details | Diff | Splinter Review
23.37 KB, patch
Details | Diff | Splinter Review
156.03 KB, patch
Details | Diff | Splinter Review
100.83 KB, patch
Details | Diff | Splinter Review
2.65 KB, patch
Details | Diff | Splinter Review
749 bytes, patch
Details | Diff | Splinter Review
45.27 KB, patch
Details | Diff | Splinter Review
158.78 KB, patch
Details | Diff | Splinter Review
103.95 KB, patch
Details | Diff | Splinter Review
165.87 KB, patch
Details | Diff | Splinter Review
110.87 KB, patch
Details | Diff | Splinter Review
172.19 KB, patch
Details | Diff | Splinter Review
117.20 KB, patch
Details | Diff | Splinter Review
195.72 KB, patch
Details | Diff | Splinter Review
140.78 KB, patch
Details | Diff | Splinter Review
1.58 KB, patch
Details | Diff | Splinter Review
1.64 KB, patch
Details | Diff | Splinter Review
1.69 KB, patch
Details | Diff | Splinter Review
1.26 KB, patch
Details | Diff | Splinter Review
821 bytes, patch
Chris Waterson
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

17 years ago
Summary says it all.  Code-level breakdown soon.

/be
(Assignee)

Comment 1

17 years ago
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

Updated

17 years ago
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

Comment 3

17 years ago
is the work for this on track?
(Assignee)

Comment 4

17 years ago
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
(Assignee)

Updated

17 years ago
Keywords: mozilla0.9 → mozilla0.8.1
Target Milestone: mozilla0.9 → mozilla0.8.1
(Assignee)

Comment 5

17 years ago
Created attachment 26760 [details] [diff] [review]
step 1: fix JS XDR bugs
(Assignee)

Comment 6

17 years ago
Created attachment 26762 [details] [diff] [review]
diff -wu version of last attachment
(Assignee)

Comment 7

17 years ago
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
(Assignee)

Comment 8

17 years ago
Created attachment 26778 [details] [diff] [review]
a few more nits picked and XXXbe's removed
(Assignee)

Comment 9

17 years ago
Created attachment 26779 [details] [diff] [review]
diff -wu version of last attachment
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.

Comment 11

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

Comment 12

17 years ago
sr=hyatt
(Assignee)

Comment 13

17 years ago
- 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
(Assignee)

Updated

17 years ago
Blocks: 31003
(Assignee)

Comment 14

17 years ago
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
(Assignee)

Comment 15

17 years ago
Created attachment 26827 [details] [diff] [review]
best patch yet
(Assignee)

Comment 16

17 years ago
Created attachment 26829 [details] [diff] [review]
diff -wu for you curious reviewers
(Assignee)

Comment 17

17 years ago
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

Updated

17 years ago
Blocks: 7251

Updated

17 years ago
Blocks: 71373

Updated

17 years ago
Blocks: 71781

Updated

17 years ago
No longer blocks: 7251

Comment 18

17 years ago
rest of this is for 0.9, correct?
Target Milestone: mozilla0.8.1 → mozilla0.9
(Assignee)

Comment 19

17 years ago
Right.

/be
Keywords: mozilla0.8.1 → mozilla0.9

Comment 20

17 years ago
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.

Updated

17 years ago
No longer blocks: 71373
(Assignee)

Comment 21

16 years ago
Not going to make 0.9, and not worth rushing.

/be
Keywords: mozilla0.9 → mozilla0.9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 22

16 years ago
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?

Comment 24

16 years ago
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...

Updated

16 years ago
Whiteboard: [nav+perf]
(Assignee)

Comment 25

16 years ago
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

Updated

16 years ago
Target Milestone: mozilla0.9.2 → mozilla0.9.3
(Assignee)

Updated

16 years ago
Blocks: 7251
(Assignee)

Comment 26

16 years ago
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
(Assignee)

Comment 27

16 years ago
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
(Assignee)

Comment 28

16 years ago
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

Comment 29

16 years ago
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.
(Assignee)

Comment 30

16 years ago
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

Comment 31

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

Comment 32

16 years ago
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?
(Assignee)

Comment 33

16 years ago
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

Comment 34

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

Comment 35

16 years ago
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).

Comment 36

16 years ago
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. 
(Assignee)

Comment 37

16 years ago
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

Comment 38

16 years ago
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).
(Assignee)

Comment 39

16 years ago
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
(Assignee)

Comment 40

16 years ago
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

Comment 41

16 years ago
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.

Comment 42

16 years ago
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).
(Assignee)

Comment 43

16 years ago
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

Comment 44

16 years ago
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.

Comment 45

16 years ago
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.
(Assignee)

Comment 46

16 years ago
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

Comment 47

16 years ago
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].
(Assignee)

Comment 48

16 years ago
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

Comment 49

16 years ago
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.
(Assignee)

Comment 50

16 years ago
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

Comment 51

16 years ago
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.  =)

Comment 52

16 years ago
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.
(Assignee)

Comment 53

16 years ago
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!!!!!!!!!!!!!!!!")

Comment 54

16 years ago
Hey Kirk. Are you holding out on a change to nsFastLoadFile.h, like the 
declaration of mSkipOffset?
(Assignee)

Comment 55

16 years ago
(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

Comment 56

16 years ago
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

Comment 57

16 years ago
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.

Comment 58

16 years ago
Adam, you should probably update in content too.

Comment 59

16 years ago
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. 
(Assignee)

Comment 60

16 years ago
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

Comment 61

16 years ago
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.

Comment 62

16 years ago
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..?

(Assignee)

Comment 63

16 years ago
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

Comment 64

16 years ago
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!'

Comment 65

16 years ago
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!

(Assignee)

Comment 66

16 years ago
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

Comment 67

16 years ago
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.

Comment 68

16 years ago
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.
(Assignee)

Comment 69

16 years ago
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
(Assignee)

Comment 70

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

/be

Comment 71

16 years ago
Do we know how smfr was measuring this? I'm guessing with the Apple profiling
tool...
(Assignee)

Comment 72

16 years ago
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
(Assignee)

Comment 73

16 years ago
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

Comment 74

16 years ago
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.

Comment 75

16 years ago
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.

Comment 76

16 years ago
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.
(Assignee)

Comment 77

16 years ago
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
(Assignee)

Comment 78

16 years ago
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
(Assignee)

Comment 79

16 years ago
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
(Assignee)

Comment 80

16 years ago
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
(Assignee)

Comment 81

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

/be
(Assignee)

Comment 82

16 years ago
Created attachment 43109 [details] [diff] [review]
diff -wu of trunk joined with branch (with fixes to undo earlier bad merging in nsHashtable.*)
(Assignee)

Comment 83

16 years ago
Created attachment 43163 [details] [diff] [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
Created attachment 43185 [details]
partial review comments
(Assignee)

Comment 85

16 years ago
Created attachment 43191 [details] [diff] [review]
revisions based on dbaron comments
(Assignee)

Comment 86

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

/be

Comment 87

16 years ago
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?
(Assignee)

Comment 89

16 years ago
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

Comment 90

16 years ago
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?


Comment 91

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

Comment 92

16 years ago
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

Comment 93

16 years ago
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? 

Comment 94

16 years ago
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.
(Assignee)

Comment 95

16 years ago
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

Comment 96

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

Comment 97

16 years ago
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?
(Assignee)

Comment 98

16 years ago
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

Updated

16 years ago
Summary: precompile chrome JS and load it incrementally → precompile chrome JS and load it incrementally ("fastload")
(Assignee)

Comment 99

16 years ago
Created attachment 44359 [details] [diff] [review]
trunk-based patch to get FastLoad working again

Comment 100

16 years ago
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.

Comment 101

16 years ago
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 ?
(Assignee)

Comment 102

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

/be
(Assignee)

Comment 103

16 years ago
Created attachment 44502 [details] [diff] [review]
slightly nicer patch, avoids an extra static helper wrapper for the nsSystemPrincipal singleton generic factory "constructor"
(Assignee)

Comment 104

16 years ago
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
(Assignee)

Comment 105

16 years ago
Sorry for the spam.

/be
Depends on: 92859

Updated

16 years ago
No longer blocks: 7251

Updated

16 years ago
Blocks: 7251

Updated

16 years ago
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 106

16 years ago
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
(Assignee)

Updated

16 years ago
No longer depends on: 93792
Adding to CC - sorry for the spam.

Comment 108

16 years ago
This bug has mozilla0.9.2 as a keyword. Update or remove?
(Assignee)

Comment 109

16 years ago
Created attachment 45892 [details] [diff] [review]
big patch to get checksumming and dependency tracking going -- don't review, you want the diff -wu version -- but do apply it
(Assignee)

Comment 110

16 years ago
Created attachment 45893 [details] [diff] [review]
diff -wu version of last patch, plesae review this while you test the last one
(Assignee)

Comment 111

16 years ago
Created attachment 45894 [details] [diff] [review]
proposed checkin comments
(Assignee)

Comment 112

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

/be
(Assignee)

Comment 113

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

/be
(Assignee)

Comment 114

16 years ago
Created attachment 45928 [details] [diff] [review]
forgot xpcom/io/nsFastLoadService.h in the patches, d'oh!
(Assignee)

Comment 115

16 years ago
Created attachment 45934 [details] [diff] [review]
replacement diffs for xpcom/io/nsFastLoadFile.{cpp,h} and xpcom/io/nsFastLoadService.cpp
(Assignee)

Comment 116

16 years ago
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

Comment 117

16 years ago
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?
(Assignee)

Comment 118

16 years ago
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
(Assignee)

Comment 119

16 years ago
Created attachment 46028 [details] [diff] [review]
all-in-one patch, sorry for the replacement patches
(Assignee)

Comment 120

16 years ago
Created attachment 46029 [details] [diff] [review]
diff -wu version of last all-in-one patch
(Assignee)

Comment 121

16 years ago
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
(Assignee)

Comment 122

16 years ago
Created attachment 46180 [details] [diff] [review]
latest and greatest all-in-one patch, for tonight's trunk (boy do i want to land this!)
(Assignee)

Comment 123

16 years ago
Created attachment 46181 [details] [diff] [review]
diff -wu version of latest patch, please review this one, it's ready to go!
(Assignee)

Comment 124

16 years ago
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

Comment 125

16 years ago
r/sr=waterson
(Assignee)

Comment 126

16 years ago
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
(Assignee)

Comment 127

16 years ago
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
(Assignee)

Comment 128

16 years ago
Created attachment 46200 [details] [diff] [review]
final patch, for application rather than review
(Assignee)

Comment 129

16 years ago
Created attachment 46201 [details] [diff] [review]
diff -wu version of last patch
(Assignee)

Comment 130

16 years ago
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.
(Assignee)

Comment 132

16 years ago
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).

Comment 135

16 years ago
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.

Comment 137

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

Comment 138

16 years ago
Please note the comment in bug 95888.
(Assignee)

Comment 139

16 years ago
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

Comment 140

16 years ago
brendan:
looks like you are really close. can you make it for 0.9.4?  :-)
(Assignee)

Comment 141

16 years ago
Created attachment 46501 [details] [diff] [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)
(Assignee)

Comment 142

16 years ago
Created attachment 46510 [details] [diff] [review]
diff -wu version of last patch

Comment 143

16 years ago
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. 

Comment 144

16 years ago
> 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).

Comment 145

16 years ago
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.
(Assignee)

Comment 146

16 years ago
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

Comment 147

16 years ago
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.
(Assignee)

Comment 148

16 years ago
/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

Comment 149

16 years ago
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.
(Assignee)

Comment 150

16 years ago
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

Comment 151

16 years ago
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).
(Assignee)

Comment 152

16 years ago
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

Comment 153

16 years ago
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.

Comment 154

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

Comment 155

16 years ago
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.

Updated

16 years ago
Depends on: 96388

Comment 156

16 years ago
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).

Comment 157

16 years ago
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.
(Assignee)

Comment 158

16 years ago
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
(Assignee)

Comment 159

16 years ago
Created attachment 46744 [details] [diff] [review]
proposed fix to avoid open-file-remove scenario that works on Unix only
(Assignee)

Comment 160

16 years ago
Created attachment 46807 [details] [diff] [review]
argh, last patch sucked, jrgm rules; try this for better file removal on bad checksum or out of date fastload file
(Assignee)

Comment 161

16 years ago
Created attachment 46840 [details] [diff] [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

Comment 162

16 years ago
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.

Comment 163

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

Comment 164

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

Comment 165

16 years ago
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

Comment 166

16 years ago
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.

Comment 167

16 years ago
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.

Comment 168

16 years ago
<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>
(Assignee)

Comment 169

16 years ago
Created attachment 47280 [details] [diff] [review]
fix so FastLoad file gets recreated after remove, in same session

Comment 170

16 years ago
sr=waterson

Comment 171

16 years ago
r=jrgm
(Assignee)

Comment 172

16 years ago
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

Comment 173

16 years ago
As per discussion with jrgm, changing QA contact to him -
QA Contact: pschwartau → jrgm
(Assignee)

Comment 174

16 years ago
Created attachment 52847 [details] [diff] [review]
turn on fastload

Comment 175

16 years ago
Comment on attachment 52847 [details] [diff] [review]
turn on fastload

sr=waterson
Attachment #52847 - Flags: superreview+

Comment 176

16 years ago
r=hyatt
(Assignee)

Comment 177

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 178

15 years ago
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.

Comment 179

15 years ago
VERIFIED since fastload cached chrome has been present for over a year.
Status: RESOLVED → VERIFIED

Updated

12 years ago
Flags: testcase-

Updated

12 years ago
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.