Closed Bug 62651 Opened 24 years ago Closed 24 years ago

Put session history on a diet

Categories

(Core :: DOM: Navigation, defect, P2)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: adamlock, Assigned: radha)

References

Details

(Keywords: memory-footprint)

Attachments

(5 files)

patch to curb SH growth
7.69 KB, patch
Details | Diff | Splinter Review
Better patch
9.86 KB, patch
Details | Diff | Splinter Review
Path with Judson's comments
10.50 KB, patch
Details | Diff | Splinter Review
Complete set that implements Jud's suggestions
11.12 KB, patch
Details | Diff | Splinter Review
Another set
12.32 KB, patch
Details | Diff | Splinter Review
It looks like session history is coming out as a pretty big ticket item in terms of how much memory it's taking up over time. In fact, it's second only to imagelib in terms of how much it's contributing to our process size growth! See these posts, for example: news://news.mozilla.org/3A2EABB5.2060403%40netscape.com news://news.mozilla.org/3A2CAD49.4020303%40netscape.com Essentially, these posts show session history is an unbounded list and grows without limits. In long browsing sessions, the history adds noticeably to the overall memory bloat. A bunch of ideas have been bandied about, including * saving a limited number of URLs in the history * truncating history by visit time, instead of count * multi-level session history (e.g., form state for last n URLs only) * tunable settings via prefs * memory pressure observer - flush when the memory is low * tie-in with session history persistence work
Keywords: footprint
See related bug 62713, which deals with shrinking the size of each nsSHEntry.
*** Bug 62956 has been marked as a duplicate of this bug. ***
nav triage team: p2 nsbeta1.
Keywords: nsbeta1
Priority: P3 → P2
Blocks: 44352
nom 0.8. Two things: 1. session history (and global history for that matter) need to be observers of the memory pressure topic. When they get memory pressure notification, they need to "shrink". A first cut at "shrinking" might be just to zero out. 2. max number of entries should be a pref setting.
Keywords: mozilla0.8
The attached patch has not been built on linux yet.
nsSHistory::~nsSHistory() has wierd indentation. actually the entire patch does. +nsSHistory::PurgeHistory(PRInt32 aEntries) + while (cnt < aEntries) { this smells like a for loop, however due to features of bad c++ compilers i can understand if you don't rewrite it as such. + cnt++; I think ++cnt; is better in c++ because you don't ask strict compilers to save the old value of cnt. + if ( urlBarHist.count == 0 ) if (!urlBarHist.count) + <titledbox orient="vertical"> + <title><text value="&shistory.label;"/></title> odd indentation, and also odd syntax.
Keywords: patch, review
nsSHistory::~nsSHistory releases the globally-held service reference once only, nulling gPrefs, but nsSHistory::nsSHistory gets the service on every constructor call. Doesn't that result in a leak? In any event, holding a service until a destructor is called can be bad at shutdown. Who calls this destructor? timeless is right, the indentation is a mess. Please convince your editor to expand tabs, and follow the Emacs modeline (which says that the indentation unit is two spaces, not 3, 4, or 7 ;-). However, I don't agree with his nit-picking (I pick nits too, that's not the problem): - PRInt32 cnt; for (cnt = 0; cnt < aEntries; cnt++) { ... } is better than the while, and the uninitialized PRInt32 cnt declaration has to go outside the for header, but so what? - cnt++ is no worse than ++cnt for any compiler worth a damn, because the value is discarded. In the case of C++ STL-like iterators, prefix ++ wins even when the value is discarded, but here we are dealing with an int. True, you might just use prefix-++ for all cases where the value is discarded, whether int or operator-overloaded object type. But I think that's a matter of individual taste, so what radha wrote (cnt++;) should stand, whether in a while loop or in the proposed for loop header. - Testing whether an int is non-zero can be done with !, but I think that's not as good a style as one that reserves ! for boolean and pointer operands. A counter brings to mind a number line, not a true/false or invalid address or valid pointer set of values. Again, this is such a small thing that I would not dictate ! over == 0 for integral types. What radha wrote there is fine. Having picked nits on top of nits, we should give the patch a closer look for more substantive problems (although it's not so easy to read without better indentation). /be
- _elementIDs = ["histDay"]; + _elementIDs = ["histDay", "shistMax"]; And as long as your touching this, please declare _elementIDs and kill another strict warning.
errr, you're. + var urlBarHist = nsJSComponentManager.getService ("@mozilla.org/browser/urlbarhistory;1", + "nsIUrlbarHistory"); + if ( urlBarHist ) How often will this fail? Let's remove this check. + { + var button = document.getElementById("ClearUrlBarHistoryButton"); + if ( urlBarHist.count == 0 ) + { + button.setAttribute("disabled","true"); + } + else + { + button.removeAttribute("disabled"); + } + } This can be shortened to button.disabled = urlBarHistory.count == 0; +/* + function onOK() + { + dump("In onOK\n"); I know this is commented out, but please remove these dump()s. We're trying to get rid of all of these kinds of dump()s that fill up the console in release builds. + var SHistory = nsJSComponentManager.getService ("@mozilla.org/browser/shistory;1", + "nsISHistory"); + var button = document.getElementById('shistMax'); + var size = button.value; + if ( SHistory ) Don't need this check. + { + dump("Setting Max SHistory length to " + maxSize); Remove this also, please. + SHistory->SetMaxLength(maxSize); maxSize?? Don't know where that came from, but it's commented out, so...*shrug* Why do we need this onOK function and the commented doSetOKCancel() above it?
I agree strongly with Brendan. >+pref("browser.session_history_max_entries", 9); Also, the default appears to be 9 history objects, no? That seems VERY small to me. The number we limit to is 100, though that's probably high. I'd suggest something more like 40 or 60. IMHO.
Urgh., I realise I added the patch while I was in the middle of debugging with a small pref value (9) and toying with the onOK() method. obviously, I have more work to do on the patch.
IIRC, MozillaClassic pruned history at 50 entries. /be
IIRC that's the limit (50) I saw in 4.x too while testing it's session history...
The latest patch takes care of the following things: 1) Builds on linux 2) History Initial size set to 50 3) I figured what the problem was with the OK callback function in pref-history.xul. So that code has been uncommented and cleaned up 4) Some of the already existing tab problems in nsSHistory.cpp fixed. 5) No need for the pref service to be a global static member. Fixed that part. Works well for the most part. Not sure about the nit-pickers though. Can I get some reviews and super reviews. Thanks,
Status: NEW → ASSIGNED
Please use the simplified button.disabled as outlined in my 01-10 comment. Couple comments on the new patch: + if ( urlBarHist.count == 0 ) Don't pad the condition with spaces, please (not done elsewhere in the same file). How often will retrieval of the nsIUrlbarHistory service fail? If not often, please remove the try/catch. We're trying to cut down on unnecessary dump()s, and in the rare case that this retrieval would fail (unless I'm missing something), an exception would be far more useful than an enduser-readable failure message. + var button = document.getElementById('shistMax'); Why is this variable called `button' if it holds a textfield?
Blocks: 64833
couple suggestions. 1. "browser.sessionhistory_max_entries" - maybe that should be "browser.history.session_max_entries" to delineate the diff between session and global? 2. You retrieve the pref service in the shistory constructor. XPCOM component constructors really shouldn't do more than init the ref count. You should add an init method, which returns an nsresult, to the shistory class and do that work in there so error code can be pushed out if need be. You'll also need to change your factory constructor to reflect the init (http://lxr.mozilla.org/mozilla/source/xpfe/components/shistory/src/nsSHistoryMo dule.cpp#37 -> NS_GENERIC_FACTORY_CONSTRUCTOR_INIT())
Oops, forgot to add the nsSHistoryModule.cpp diffs to the recent patch. It follows
There is no pattern right now in the prefs files in differentiating global history from session Hsitory. I thought browser.history.session_max_entries was little confusing. So, I made the SH pref to browser.sessionhistory.max_entries. May be all SH related prefs in the future can follow this pattern.
Please address my comments as well... Or should I just post a patch that does so (for that file)?
bug 33115 patch 2 is killing ghist.* entries. has anyone ever heard of them?
Blake, I think the try {} .. catch{} is defensive coding and I intend to keep it. I'll probably take the dump() off. Regarding the way the "Clear urlbar history" is disabled, I think what is in there right now is more easy to understand than the suggestion you made. I'll probably do these changes before I checkin. I may not necessarily attach a patch for these. I don't think pruning of ghist.* prefs from all.js has any impact on this bug or the patch attached here.
Should the session history be listening for changes to the preference if we intend for the user to be able to change it at runtime? Is it also possible to check the user-supplied value so that stupid values such as -1000 are rejected?
Forget the first issue, the code in onHistoryOK should cover that... The second question still stands though.
On the second question, I can add code to nsSHistory::SetMaxSize() to ignore such values, programatically. But should we also provide UI feedback in the pref panel for such values? Should there be a max limit too? What if the user typed 10000? Should there be a enforced limit of 50? ie., user can not enter a value beyond 50?
Though I have not received any formal "r=" and sr=" comments in this bug, I have received several suggestions on the patch and I have incorporated most of them to the extent possible. The patch has been around for about 6 days and I think the performance team would like to use it in their daily perf tests. So, I intent to checkin what I have unless anybody has major objections to it.
radha: could you please post the latest patch that you intend to check in?
Only one nitpicky comment: - In nsSHistory::Init(), don't use `bare' nsServiceManager. Instead, use scc's newer form with a ContractID: nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID); if (prefs) { ... } Fix that, and sr=waterson. Somebody more familiar with session history (law? alecf?) should r= one more time, too. thanks!
radha: the try {...} catch (ex) {} (empty catch) usage may be the wrong thing, as blake tried to point out. It's not defensive, if the errors it ignores lead to further badness that isn't well-handled, and/or if the errors it ignores are not reported to someone who can repair things. For example, in Startup, if the service isn't available, shouldn't the exception propagate all the way to the native code? Also, blake pointed out a better way to write + if (urlBarHist.count == 0) + { + button.setAttribute("disabled","true"); + } + else + { + button.removeAttribute("disabled"); + } IIRC, he suggested button.disabled = (urlBarHist.count == 0) -- does that not work? If it does, please use it. Otherwise, can someone school me on why one must *remove* an attribute to (re-)enable an input element? /be
r=law "cvs diff -w" would have helped in this case, BTW. One thing that threw me for a moment was the fact that you added a maxLength attribute to the interface but each instance actually shares a single, global attribute. I wanted to think that each SH object had it's own max length. Not much you can do about that, I suppose. nsISHistory isn't a service, is it? There's a unique instance per browser window, right? It seems odd for a random instance to be created by the pref-panel just to get an object to call SetMaxLength through. I guess that's the flip side to making that a per-instance attribute when really it's not. Would a pref change callback be better?
There look to be a couple of sloppy button rules (cd mozilla/themes ; find . -name \*.css -print | xargs grep -n disabled) that simply test for presence of the attribute (`button[disabled]') instead of testing for attribute and value (`button[disabled="true"]'). Radha: are you dealing with one of these rules?
be: Can you suggest a way to get a handle to a service/object and use it safely with out assuming that the creation call would have succeeded. I understand that., var SHistory = nsJSComponentManager.getService(.....); if (SHistory) {.....} isn't the best thing to do either. Sometime ago, removeAttribute() was the way to disable a attribute rather than test for its value. That has changed. I don't believe there is anything wrong with blake's suggestion.
Assuming that it succeeded is probably the best way to go, unless there's reasonable probability that it will fail. After all, an exception is far more useful than a human-readable "***couldn't retrieve service" failure. In this case, I can't think of any reason why the nsIUrlBarHistory interface would frequently be unavailable. The debate about whether to remove attributes or just negate them has been going on for awhile, but I don't think a definitive conclusion was ever reached. Some people argue that the attribute should be removed entirely, because it's only an attribute of the element if it's true. Others argue that the negated attribute, e.g. NOT being hidden, is also a property of the element. Nowadays, I think we generally prefer removing the attribute instead of setting false, although we still do the latter (inconsistently) in many places, which is often dangerous. XBL-defined properties supercede the need for this debate and make attributes easier to read and modify. "disabled" is a <property/> of the textfield binding, so it can be accessed directly (button.disabled instead of button.set/getAttribute()). It's typically just a form of shorthand, though, since the property [s/g]etters usually just call set/getAttribute. So accessing the attribute directly if it's defined in XBL is almost always preferable to using set/getAttribute.
The last patch, with the pref set to `8', plus the patch for bug 65908, reduce the gross VM growth rate with the lea-pre7 allocator from 10.1KB/URL to 5.8KB/URL. After loading 500 pages, that's a savings of about 2MB of virtual memory footprint. Nice going!
I checked in the latest patch with little more nit-pick comments taken care of. Shall try to address some of the pref UI improvements(like ensuring illegal values are not saved in prefs)in a later checkin.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
whoo! hoooo! thanks for working on this radha. sounds like it's going to make a big difference in memory usage.
The fix for this bug adds an additional dependency upon prefs from shistory. Module dep tracking for http://bugzilla.mozilla.org/show_bug.cgi?id=59454.
Keywords: mozilla0.8
No longer blocks: 64833
why is this pref in the UI? there is absolutely no reason for it, no end user will ever need to change it, and it is yet another reason why our prefs UI is bloated and confusing. was there any feedback from UE on adding the pref UI?
This was put in because it was part of what was causing long-term heap fragmentation (and footprint growth). The newsgroup discussions referenced provide some of the background. Netscape (or whomever) could remove the UI and set the default value to 50 (or 500000, or 5) if they so wish.
why is this a netscape issue? having a reasonable default obviates the need for a user-level pref. keep it hidden for tweaking. no end user, mozilla or otherwise, would ever want to change this. if they really do, they can edit prefs.js. this is typical of why our prefs are a mess. mozilla is not a geek browser. once it becomes that, we're doomed.
I forget the discussions (which were in the newsgroup), but I think people had wildly different ideas of what was a 'good' value. Honestly I wouldn't care if the UI were to go away so long as the pref remained and the default was "reasonable" (say at least 50). I suspect the UI got created because it was easy and people wanted to play with it. However, I don't see this as a major minus to mozilla. Mozilla _is_ in many ways a geek browser; but one that's used as the base for a number of very much non-geek browsers (Netscape, etc). Someone making a mass-market browser from Mozilla will hide as least as many things as are added I'd think; probably more.
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: