Currently only use .autoreg to determine if we should rebuild the component registries and do the double startup thing. We should extend .autoreg to control cache-consistency of other areas such as javascript components. For example: currently we stat() every js component to see if it matches the data in the fastload stuff, should instead of reusing .autoreg for that.

Well, actually we do remove the fastload file on updates: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2345 So it's then a question of whether we trust .autoreg (and the version-compatibility checking) to be good enough that we don't have to do internal fastload invalidation.

Perhaps stat() just the directory, and see if its mtime changed?

Its mtime will always have changed... the history database lives there.

I meant statting $appdir/components/... I'm not familiar with this code (so this could all be nonsense!), just saying that 1 stat of the dir would be faster that lots of per-file stats.

(In reply to comment #4) > I meant statting $appdir/components/... I'm not familiar with this code (so > this could all be nonsense!), just saying that 1 stat of the dir would be > faster that lots of per-file stats. of course it might be easiest to just combine components into one file, bug 507038

Note that fastload invalidation is *mostly* concerned with extension development practices, where the application has not changed at all. It should be completely unnecessary to check invalidation data on anything in the appdir.

Of course, many of those things are a PITA for developing, where having the JS compaonents symlinked to the actual source files saves rebuild steps and allows for rapid develop/test cycles. I sincerely hope we're doing all those agglomeration steps either optionally or only at packaging.

i filed bug 512827 for removal of the checks of individual files, to distinguish that from this more general bug, and since it's not actually .autoreg related. (In reply to comment #7) > Of course, many of those things are a PITA for developing, where having the JS > compaonents symlinked to the actual source files saves rebuild steps and allows > for rapid develop/test cycles. I sincerely hope we're doing all those > agglomeration steps either optionally or only at packaging. we should avoid making development more difficult, where possible. however, we should absolutely not withhold performance improvements from hundreds of millions of users for it.

(In reply to comment #1) > Well, actually we do remove the fastload file on updates: > http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2345 > > So it's then a question of whether we trust .autoreg (and the > version-compatibility checking) to be good enough that we don't have to do > internal fastload invalidation. It looks to me that will remove the XUL fastload but not the XPC fastload. (In reply to comment #2) > Perhaps stat() just the directory, and see if its mtime changed? This won't work with some filesystems (FAT for example and Mac OS X under some circumstances) since they won't update the directory's mtime when files within the directory have changed. Seems like adding the removal of the XPC fastload and removing the fastload checks for file changes would resolve this bug overall. For developing would it be possible to always invalidate the fastload files on startup? Perhaps via a pref file that isn't added to the package but is overridden by the test harnesses?

(In reply to comment #9) > (In reply to comment #1) > > Well, actually we do remove the fastload file on updates: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2345 > > > > So it's then a question of whether we trust .autoreg (and the > > version-compatibility checking) to be good enough that we don't have to do > > internal fastload invalidation. > It looks to me that will remove the XUL fastload but not the XPC fastload. or perhaps fastload invalidates the XPC fastload file when the XUL fastload isn't present?

<ddahl> bsmedberg: just noticed you are not cc'd on 511761 <ddahl> would love a comment about rs comments <ddahl> its an important bug for firefox startup <bsmedberg> we should just drop .autoreg altogether... it's sucky and doesn't integrate with apprunner <bsmedberg> compatibility.ini or whatever it's called is the primary "reregister the world" mechanism <Mossop> We still need a way for the extension manager to signal that a re-register is necessary <Mossop> .autoreg is currently that <bsmedberg> Mossop: why not just... delete compreg.dat or hand a flag back to apprunner to do something in compatibility.ini? <Mossop> bsmedberg: Yeah we can do that

Removes some stat calls, attempts to use compatibility.ini and compreg.dat in place of checking last-modified-time and .autoreg. I could really use some comments on this patch, or suggestions on how to test it. A lot of the changes involve simply removing tests that seemed overly defensive, but I could easily have misunderstood the execution path, etc.

Some numbers using the method posted here: http://blog.vlad1.com/2009/07/28/measuring-startup/ Without patch: 788 832 777 806 782 777 792 787 789 782 775 785 With patch: 844 792 775 761 781 765 764 778 773 760 771 813 760

An additional report from the department of questionable statistics: Cold start: Without patch: 13702 8851 11140 10669 10280 10407 10573 Mean: 10803 With patch: 13773 10361 10218 8465 9821 11194 10638 12594 10441 Mean: 10833 --------- Warm(?) start (same numbers as above): Without patch: 788 832 777 806 782 777 792 787 789 782 775 785 Mean: 789 With patch: 844 792 775 761 781 765 764 778 773 760 771 813 760 Mean: 779

Comment on attachment 401339 [details] [diff] [review] removes some stat() calls, particularly some on .autoreg > const FILE_AUTOREG = ".autoreg"; leftover? > const FILE_INSTALL_MANIFEST = "install.rdf"; > const FILE_CHROME_MANIFEST = "chrome.manifest"; >+const FILE_COMPREG = "compreg.dat"; > > // create the file. > try { >- var autoregFile = getFile(KEY_PROFILEDIR, [FILE_AUTOREG]); >+ // We don't like .autoreg anymore. >+ /*var autoregFile = getFile(KEY_PROFILEDIR, [FILE_AUTOREG]); > if (val && !autoregFile.exists()) >- autoregFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE); >+ autoregFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);*/ don't comment stuff out in patches, just delete it >+ >+ // compreg.dat is our new friend. Hello, compreg.dat. >+ // We remove this guy to signal the nsAppRunner guy to re-register. >+ var compregFile = getFile(KEY_PROFILEDIR, [FILE_COMPREG]); >+ if (val && compregFile.exists()) >+ compreg.remove(false); > } > catch (e) { > } > return val; > * Gathers data about an item specified by the supplied Install Manifest >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >--- a/toolkit/xre/nsAppRunner.cpp >+++ b/toolkit/xre/nsAppRunner.cpp >@@ -2272,21 +2272,25 @@ WriteVersion(nsIFile* aProfileDir, const > } > > static PRBool ComponentsListChanged(nsIFile* aProfileDir) > { > nsCOMPtr<nsIFile> file; > aProfileDir->Clone(getter_AddRefs(file)); > if (!file) > return PR_TRUE; >- file->AppendNative(NS_LITERAL_CSTRING(".autoreg")); >- >+ //file->AppendNative(NS_LITERAL_CSTRING(".autoreg")); >+ file->AppendNative(NS_LITERAL_CSTRING("compreg.dat")); >+ > PRBool exists = PR_FALSE; > file->Exists(&exists); >- return exists; >+ //return exists; >+ >+ // If we need a re-register, someone should have removed this file. >+ return !exists; This makes me wonder if we can't just go to try to load compreg....fail there. Instead of doing an explicit check. >- if (CheckUpdateFile() || NS_FAILED( >+ // if (CheckUpdateFile() || >+ // Let's ignore .autoreg. "re-register the world" if not compreg.dat >+ if (NS_FAILED( Don't do this. if the line is too long stick return code into nsresult rv, check that. > PRInt64 modTime = 0; >- if (NS_SUCCEEDED(aComponentFile->GetLastModifiedTime(&modTime))) { >+// if (NS_SUCCEEDED(aComponentFile->GetLastModifiedTime(&modTime))) { > PRInt64 cachedModTime; >- if (mAutoRegEntries.Get(lfhash, &cachedModTime) && >- cachedModTime == modTime) >+ if (mAutoRegEntries.Get(lfhash, &cachedModTime)) //&& >+// cachedModTime == modTime) > return NS_OK; >- } >+// } same thing re:commenting out here. >- >- PRInt64 currentMtime; >+ /*PRInt64 currentMtime; >+ fprintf(stderr, "calling getLastModifiedTime, should cause a stat...\n"); > rv = file->GetLastModifiedTime(&currentMtime); and here. >+ }*/ .... I'll play with the patch some more now

From irc discussion, this patch isn't complete yet. There are still another 100 or so component stats() left.

Also from irc discussion, I'm still really interested in suggestions for good ways to test this. Bsmedberg says he previously had difficulty figuring out ways to test xptinfo, in particular. (Next step for this patch is mucking with xptiInterfaceInfoManager). One example: >bsmedberg: multiple registrations of the same IID might behave differently or cause problems >bsmedberg: especially if they had different names >bsmedberg: bhsieh: it's very sensitive to the weird stuff plugins do

>This makes me wonder if we can't just go to try to load compreg....fail there. >Instead of doing an explicit check. It's technically possible to do this. compreg gets loaded a few lines down in the xpcom.Initialize() call. But we'd have to pass the loaded/failed-to-load up through two method calls and make sure that no one read the files-to-be-removed during the rest of initialization, or pass down a parameter telling the Initialize() call whether to remove these files or not (there are other causes for removing the files). Neither solution makes that much sense from a code-organization point of view, and it only saves one stat(). Let me know if you feel that it's really important, though.

Asking in this bug because it seems to have more readers: Are there any other cases in which the caches need to be invalidated? (According to bug 517515, they all go together.) So far, we invalidate when the buildid changes and when an extension is installed/upgraded/otherwise affected. When else do we need to dump our caches?

(In reply to comment #19) > Asking in this bug because it seems to have more readers: > > Are there any other cases in which the caches need to be invalidated? > (According to bug 517515, they all go together.) So far, we invalidate when the > buildid changes and when an extension is installed/upgraded/otherwise affected. > When else do we need to dump our caches? Probably just an oversight but also when the abi, path to the app dir, or the path to the platform dir changes.

Comment on attachment 401969 [details] [diff] [review] fixes the comments This looks good to me(other than that ->Clone() issue), I can't review this code for commit purposes. Ask bsmedberg for review if you decide to land this separately from bug 517515.

Only uses compatibility.ini to invalidate caches, not stat()s or .autoreg. Adds a (scriptable) method to nsIXULRuntime that allows callers to signal for the caches to be invalidated on the next restart. When called, the method writes a flag to compatibility.ini. On application start, we check for that flag and if we find it we invalidate the caches and remove the flag. Currently the only caller of this method is the extensions manager.

to clarify: also addresses the comments on the patch in bug 517515

Comment on attachment 403872 [details] [diff] [review] folds in patch from 517515 >diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in >+const FILE_COMPREG = "compreg.dat"; Is this a leftover constant? It doesn't appear to be used anywhere in this patch. >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >+NS_IMETHODIMP >+nsXULAppInfo::InvalidateCachesOnRestart() >+{ >+ nsCOMPtr<nsIFile> file; >+ nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, >+ getter_AddRefs(file)); This should be NS_APP_PROFILE_DIR_STARTUP. I'm pretty sure that when the extension manager calls this function the profile dir is not yet available and so the PROFILE_50_DIR key will fail. >+ nsCOMPtr<nsILocalFile> localFile(do_QueryInterface(file)); >+ nsINIParser parser; >+ rv = parser.Init(localFile); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ nsCAutoString buf; >+ rv = parser.GetString("Compatibility", "InvalidateCaches", buf); >+ >+ if (NS_FAILED(rv)) { >+ PRFileDesc *fd = nsnull; >+ localFile->OpenNSPRFileDesc(PR_RDWR | PR_APPEND, 0600, &fd); >+ if (!fd) { >+ NS_ERROR("could not create output stream"); >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ static const char kInvalidationHeader[] = NS_LINEBREAK "InvalidateCaches=1" NS_LINEBREAK; >+ rv = PR_Write(fd, kInvalidationHeader, sizeof(kInvalidationHeader) - 1); >+ PR_Close(fd); If compatibility.ini doesn't exist, this will create it but it won't have any section headers. I think that if compatibility.ini doesn't exist, you shouldn't create it here. >+// Checks the compatibility.ini file to see if we have updated our application >+// or otherwise invalidated our caches. If the application has been updated, >+// we return PR_FALSE. Otherwise, we return PR_TRUE and write the status of the >+// caches (valid/invalid) into return param aCachesOK. >+// Note: we do not write into aCachesOK in the case of an application update, >+// although this generally does also require a cache invalidation. > static PRBool > CheckCompatibility(nsIFile* aProfileDir, const nsCString& aVersion, > const nsCString& aOSABI, nsIFile* aXULRunnerDir, >- nsIFile* aAppDir) >+ nsIFile* aAppDir, PRBool* aCachesOK) Why don't you write aCachesOK? Please make the comment a doccomment instead of a //-style comment. > static void RemoveComponentRegistries(nsIFile* aProfileDir, nsIFile* aLocalProfileDir, > PRBool aRemoveEMFiles) You didn't modify this function at all, but currently it only deletes XUL.mfasl, not XPC.mfasl or whatever the JS component fastload file is called. Since you're removing the fastload timestamp checks you absolutely need to make sure this function does everything it needs to. >diff --git a/xpcom/io/nsFastLoadFile.cpp b/xpcom/io/nsFastLoadFile.cpp >- PRInt64 currentMtime; >- rv = file->GetLastModifiedTime(&currentMtime); >- if (NS_FAILED(rv)) >- return rv; >- >- if (LL_NE(fastLoadMtime, currentMtime)) { >-#ifdef DEBUG >- nsCAutoString path; >- file->GetNativePath(path); >- printf("%s mtime changed, invalidating FastLoad file\n", >- path.get()); >-#endif >- return NS_ERROR_FAILURE; >- } Removing this hunk is going to hurt local developers. I'm pretty sure you should keep it, at least for DEBUG builds.

>Is this a leftover constant? It doesn't appear to be used anywhere in this >patch. Yes, removed now. >This should be NS_APP_PROFILE_DIR_STARTUP. I'm pretty sure that when the >extension manager calls this function the profile dir is not yet available and >so the PROFILE_50_DIR key will fail. Fixed. >If compatibility.ini doesn't exist, this will create it but it won't have any >section headers. I think that if compatibility.ini doesn't exist, you shouldn't >create it here. Okay, now checks for that. I'm not sure what to do in this case, though -- I have the method returning an error but I don't know what recourse callers will have. compatibility.ini gets written out pretty early on, though. >Why don't you write aCachesOK? Changed now to write it out regardless. Originally I thought of the function as returning error codes, so I didn't want to write a return param on error. >Please make the comment a doccomment instead of a //-style comment. Done. >You didn't modify this function at all, but currently it only deletes >XUL.mfasl, not XPC.mfasl or whatever the JS component fastload file is called. >Since you're removing the fastload timestamp checks you absolutely need to make >sure this function does everything it needs to. Now removes XPC.mfasl as well. >Removing this hunk is going to hurt local developers. I'm pretty sure you >should keep it, at least for DEBUG builds. Now keeping it for debug builds.

Comment on attachment 405598 [details] [diff] [review] addresses above comments >diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp >+NS_IMETHODIMP >+nsXULAppInfo::InvalidateCachesOnRestart() >+ file->AppendNative(FILE_COMPATIBILITY_INFO); >+ PRBool exists; >+ if (NS_FAILED(file->Exists(&exists)) || !exists) { >+ return NS_ERROR_FILE_NOT_FOUND; >+ } I don't think you need to explicitly check its existence. calling parser.Init below will fail if the file doesn't exist, and you can just return from there. >+ nsCOMPtr<nsILocalFile> localFile(do_QueryInterface(file)); >+ nsINIParser parser; >+ rv = parser.Init(localFile); >+ if (NS_FAILED(rv)) >+ return rv; If you can't read the file, you might as well return NS_OK from this function: on the next restart we'll flush the caches anyway. >diff --git a/xpcom/components/nsComponentManager.cpp b/xpcom/components/nsComponentManager.cpp > PRInt64 modTime = 0; >- if (NS_SUCCEEDED(aComponentFile->GetLastModifiedTime(&modTime))) { >- PRInt64 cachedModTime; >- if (mAutoRegEntries.Get(lfhash, &cachedModTime) && >- cachedModTime == modTime) >- return NS_OK; >- } >+ PRInt64 cachedModTime; >+ >+ // If it's in the cache, it should be valid. >+ // The cache file is removed if files are modified. >+ if (mAutoRegEntries.Get(lfhash, &cachedModTime)) >+ return NS_OK; Sorry I missed this the first time around: this block should remain as-is for debug builds, so something like: if (mAutoRegEntries.Get(lfhash, &cachedModeTime)) { #ifdef DEBUG if (NS_SUCCEEDED(aComponentFile->GetLastModifiedTime(&modTime)) && cachedModTime == modTime) return NS_OK; #else return NS_OK; #endif } r=me with those changes

Backed out at Dietrich's request due to failures. http://hg.mozilla.org/mozilla-central/rev/86a6cd701118

one-line addition to fix a test, addition ok'd by dtownsend (the author of the test).

(In reply to comment #30) > Created an attachment (id=406544) [details] > fixes test failure > > one-line addition to fix a test, addition ok'd by dtownsend (the author of the > test). Bad diff; that's way more than a line :-)

The interdiff only shows one https://bugzilla.mozilla.org/attachment.cgi?oldid=405903&action=interdiff&newid=406544&headers=1

Booya, noticable Ts improvements on small, medium and large dirty profiles when it relanded with the test fix.

http://hg.mozilla.org/mozilla-central/rev/1c02fe2e41e5 Booyah. Great work, Ben!

Clearing checkin-needed. How do we feel about taking this on 1.9.2?

Potential regression: bug 525154

rant I just lost a lot of time *again*, because my changes didn't take effect. I already learned to delete *.mfasl before starting TB, but I script that, because it's a long path, and I changed the profile, and was wondering why |make| is broken. Do you *have* to make XUL development a pain? There's a tradeoff between dev and runtime, and we already make a huge tradeoff by using JS and XUL instead of C++. Do we have to let new XUL developers suffer severe pain, and even cut experienced devs, for the 10 / 30 milliseconds we gained here (comment 14)? It's stuff like this which makes people drop a solution.

> It's stuff like this which makes people drop a solution. This is observation: I have seen people get very frustrated and walk away from XUL, because they couldn't get their attempts to even start. They burn an hour sticking around in the dark and walk away. Most of the time, if I diagnosed it, turned out to be component registration issues. This is a prime reason why people don't even start using XUL, from what I could see.

> I just lost a lot of time *again*, because my changes didn't take effect. I > already learned to delete *.mfasl before starting TB, but I script that, > because it's a long path, and I changed the profile, and was wondering why > |make| is broken. Are you saying that deleting *.mfasl doesn't work, or that it's painful to do? If it's the former, 1) that's kind of surprising, and 2) you could try deleting compatibility.ini instead. It lives in the other [non-local?] profile directory, which is kind of confusing at first. Or was the painful thing that you changed the profile but not your reload script? By the way, there's now a scriptable way to force reload of everything on the next restart. I don't know if that helps you, but you do it like this: + let XRE = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime); + XRE.invalidateCachesOnRestart(); This works on a per-profile basis. > Do you *have* to make XUL development a pain? There's a tradeoff between dev > and runtime, and we already make a huge tradeoff by using JS and XUL instead > of C++. Do we have to let new XUL developers suffer severe pain, and even cut > experienced devs, for the 10 / 30 milliseconds we gained here I think we did end up gaining more than 30 ms from this together with bug 510991, there's some more numbers in comment 8 there. I think that stat()s get cached, so if that's the case, then removing all of them is a much bigger win than landing either of the patches separately. I don't know what kind of tradeoffs we want in general. Maybe other people can pipe in about that. Maybe we can do more stat()ing on debug builds or something. Again other people can pipe in about whether / how quickly we should do that.

Sorry, comment link busted there, the numbers for the two patches together are here: https://bugzilla.mozilla.org/show_bug.cgi?id=510991#c8 Again I think that either patch alone doesn't produce this kind of effect.

Is there a comment that explains the changes made here? Sounds like this will need a documentation update..

Nickolay: where should I add comments/documentation for this? I don't think I ever found documentation for the old system, but maybe I didn't look in the right places... Instead of creating the .autoreg or removing compreg.dat to signal for cache invalidation, call invalidateCachesOnNextRestart() on nsIXULRuntime. We also don't stat() to determine if files have been updated anymore, which is probably what Ben Bucksh was talking about. Behind the scenes, this adds a line to the compatibility.ini file which causes cache invalidation the next time nsAppRunner.XREMain() runs.

As far as I'm concerned, nsIXULRuntime.invalidateCachesOnNextRestart is an internal function that should only be used by the extension manager. The only real documentation here should probably be taken from the information in this thread: http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/ca867015d8e35fd2#

You offer only 3 alternatives in this thread: * Use debug builds That's out for me, because they are *way* too slow (2-3 times slower), and way too spammy lots of assertions (on console and sometimes as popups) which nobody fixes, and I can't see my own output anymore. Debug builds are unusable for me. * Change version/buildID in compatibility.ini. That's good for releases, but not during development. make -C mail must work and be sufficient (if I use jar files. If I don't use jars, just changing the XUL/JS file must be sufficient). * Delete *.mfasl in my profile That works, but a) is unintuitive and will be a problem for new XUL developers (comment 38). Even many Mozilla developers (e.g. dmose) are/were not aware of this and accounted this as "strange errors". b) is error-prone. I know of this requirement, and script it / use bash history, but that is cumbersome, and I need to change this *every time* I create a new profile for testing, and I have already forgotten about this several times, leaved to lots of wasted time hunting on the wrong ends (my code, build, ...). This is extremely annoying to have to have to *manually* delete files in my profile, given the long profile and strange path, between *every* change and test (app start). We need a better solution, it can't stay like this. This is unusable for development.

(In reply to comment #44) > You offer only 3 alternatives in this thread: this is not a forum thread, it's a closed bug. can you please file a new bug that clearly states your request, and maybe push the discussion onto dev.platform? that way it would get the broader visibility from core developers that it requires.

First, to quote Taras from the cited newsgroup thread: Cheers to benh for taking this on. Second, having said that, I agree with Ben Bucksch, we need a better solution for XUL developers. I sat on my hands when the regressing patch went in, hoping the effect would be mitigated somehow. But it hasn't been. Blowing off developer concern and advising a relatively high-cost change to XUL developers' work habits is bad for our developers. Blair pointed this out in the cited thread: http://groups.google.com/group/mozilla.dev.platform/msg/b6adbc3e02261863 If we have the code around #ifdef DEBUG, why not change it to be under control of a pref, and let devs set the pref? /be Update: I mid-aired with Dietrich but I'm pushing this comment through anyway, with a +1 for Dietrich's call for a new bug. I don't know about a forum thread at this point, there's clearly a regression from the former behavior enjoyed by XUL app developers. It's a bug, let's fix it.

> let devs set the pref? Unfortunately, a pref doesn't work too well, because I keep creating new profiles. Better would be checking for existence of a file in the build dir (not pref). Both don't help new XUL developers, though. Dietrich Ayala, I think regressions that a change causes are on-topic in a bug, esp. if one possible solution is back-out. In my humble opinion, this should be backed out until we have a workable solution for all parties.

I am not "blowing off" the concerns, I believe that the changes made here are an acceptable trade-off in release builds. The primary disadvantage of a pref is that it requires us to keep a tracking mechanism in place which associates each piece of fastloaded data with a disk file. This requires additional runtime cost (to unwrap JAR/chrome/resource URIs, for example), and that we should continue this work even further so that even in DEBUG builds we have a "dumber" way of invalidating the fastload cache if you are doing active development. There already exists a pref: it turns off the fastload cache entirely, and that is exactly what BenB should be using. An additional pref is pain without real gain.

Per request, I filed new bug 531886. I already wrote by a pref doesn't work. And dev builds must still be usable, so if the "solution" is degrading that seriously, it's not acceptable.

A pref works fine, and you can edit a file in your build dir already (add something to the defaults dir, right?) if you want it to span all profiles. The cache is scoped to the profile, so the control for it should be as well. Developers already set profile prefs to get dump() output, errors for chrome, etc.

(In reply to comment #48) > I am not "blowing off" the concerns, I believe that the changes made here are > an acceptable trade-off in release builds. That's obviously true (that you believe this) but it remains the case that XUL developers do not agree, and a bug remains a bug if it bites enough people and there is a demonstrable regression in behavior. > The primary disadvantage of a pref > is that it requires us to keep a tracking mechanism in place which associates > each piece of fastloaded data with a disk file. Which IIUC is there in debug builds still. Right? > This requires additional > runtime cost (to unwrap JAR/chrome/resource URIs, for example), I remember writing that code... > and that we > should continue this work even further so that even in DEBUG builds we have a > "dumber" way of invalidating the fastload cache if you are doing active > development. Sorry, what dumber way? /be

For example, rather than statting files by timestamp and comparing them to an .autoreg timestamp (or to their former timestamp), write a file with a UUID or similar random contents each time "make" is issued in the build tree, and record/compare that in compatibility.ini to reset the fastload caches... this would be like a "development buildID" that changes each time you build.

(In reply to comment #52) > For example, rather than statting files by timestamp and comparing them to an > .autoreg timestamp (or to their former timestamp), write a file with a UUID or > similar random contents each time "make" is issued in the build tree, and > record/compare that in compatibility.ini to reset the fastload caches... this > would be like a "development buildID" that changes each time you build. Ok, sounds like this could fix bug 531886. But if it's only in DEBUG builds it is no help to XUL developers who need release build speed and accuracy (debug builds don't always behave the same, apart from performance being worse). And it's all future work and in the mean time XUL developers experience a regression. This is what I meant by "blowing off". We took a miscalculated regression with some advice (not heard by the affected developers) to add cost to work habits. I think that's bad business, always. It is a way to lose devs to competing platforms and browsers, all else equal. Faster startup does not countervail it. /be

Replying to Ben's and Ben's comments: > Nickolay: where should I add comments/documentation for this? ... >The only real documentation here should probably be taken from the information > in this thread: Assuming this stays in, many docs on developer.mozilla.org should be updated 1) mentioning .autoreg: https://developer.mozilla.org/index.php?title=Special%3ASearch&search=autoreg&language=en&fulltext=Search 2) various pages that we think are likely to be seen by new developers (e.g. for extensions https://developer.mozilla.org/en/Setting_up_extension_development_environment , various tutorials, we unfortunately have many of them) should explain what people should do in order to get their changes to {chrome xul|js|components|modules} in {extensions|themes|mozilla source code} apply to restarted Firefox 3) big announcement on the "Firefox x.y for developers" and "Updating extensions for Firefox x.y" pages detailing the changes and linking to the new best practices doc (2). (Aside - sorry for ranting: it seems obvious that a drastic change like this should at least be explained somewhere, regardless of whether the old system was fully documented. I spent an hour or so figuring out exactly what changes were made and how they're affecting workflows I'm aware of... The result is at http://docs.google.com/View?id=dfs226xt_14hkzngff7 - I can give edit access or move to wiki.m.o if someone wants to edit) But let's see what happens with bug 531886 first.

What were the "noticeable wins" by the way? If the wins are small on desktop, but big on mobile, perhaps desktop builds could keep doing a few stat extra checks?