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: