Closed Bug 329816 Opened 14 years ago Closed 11 years ago

PLT regressions from places


(Firefox :: Bookmarks & History, defect, P2)






(Reporter: brettw, Unassigned)


(Whiteboard: swag: 5d)


(2 files)

Places causes page load time to be somewhere between 4% and 9% slower in my tests. It should be as fast or faster as before places.
Priority: -- → P1
Target Milestone: --- → Firefox 2 alpha2
Depends on: 329277
No longer depends on: 329277
Whiteboard: swag: 3d
The major slowdown from places comes from AddURI. This patch buffers this call, as well as calls to set the title and the favicon. Currently, this timer is 3s, which seems to give good results. When the timer fires, we commit all pending items. This means that for fast loading pages we do this operation after the page has been shown. For pages that take > 3s to load, you won't notice because it already takes so long.

I'm not sure how much difference this will make in the gaius page load tests. Places on my system only causes a 1-3% slowdown, while gaius is currently seeing about 20%. On my system, this makes the slowdown from places about half what it was before (I'm seeing a Mork = 187ms, current places = 190ms, with patch = 188ms). Theoretically, it should be much better. When we get a slower computer set up I can test better here.
Attachment #216568 - Flags: review?(bryner)
Comment on attachment 216568 [details] [diff] [review]
Lazy AddURI patch

>@@ -130,16 +133,40 @@ public:
>+  void SyncDB()
>+  {
>+    #ifdef LAZY_ADD
>+      CommitLazyMessages();
>+    #endif

I think some compilers only allow preprocessor directives to start at column 0, so get rid of the leading whitespace.  Same elsewhere in this change.

>@@ -367,16 +398,58 @@ protected:
>+#ifdef LAZY_ADD
>+  // lazy add committing
>+  struct LazyMessage {
>+    // call this with common parms to initialize. Caller is responsible for
>+    // setting other elements manually depending on type.

Hm, I wonder if this should at least initialize all of the members to _something_.  It seems like it would be more reliable if you could add an assertion that these values are initialized to something meaningful when you go to use them (and assertions on uninitialized memory don't really work).

>--- nsNavHistory.cpp	27 Mar 2006 18:11:12 -0000	1.89
>+++ nsNavHistory.cpp	28 Mar 2006 20:28:45 -0000
>@@ -103,16 +103,30 @@
> // the value of mLastNow expires every 3 seconds
> // see bug #319004 -- clamp title and URL to generously-large but not too large
> // length
> #define HISTORY_URI_LENGTH_MAX 65536
>+#ifdef LAZY_ADD
>+  // lazy message types
>+  #define LAZY_ADD_URI 0
>+  #define LAZY_TITLE 1
>+  #define LAZY_FAVICON 2

I think this would this make more sense as an enum.

>+  for (PRUint32 i = 0; i < mLazyMessages.Length(); i ++) {
>+    LazyMessage& message = mLazyMessages[i];

Hm, can this be |const LazyMessage&| ?  I guess not if that forces the fields to be const.

Looks ok otheriwse, r=me with those fixed.
Attachment #216568 - Flags: review?(bryner) → review+
Lazy patch is on branch and trunk.
Attachment #216677 - Flags: review?(bryner)
Attachment #216677 - Flags: review?(bryner) → review+
Lazy adding temporarily disabled on trunk.
Whiteboard: swag: 3d → swag: 5d
Lazy adding re-enabled on trunk on April 8.
Target Milestone: Firefox 2 alpha2 → Firefox 2 beta1
At last look, gaius' Tp was <2% slower with places.

Here are some timings I did on my very fast Windows box by calling the corresponding function thousands of times. 

Times in ms. IsVisited with history (found and not found) were tested with 10K history items. The "typical page" load time is with 198 unvisited links, 2 visited links, 1 add old URI, 1 add new URI. This is the amount of time taken by the history service in Tp

                       Mork   sqlite  bloom  lazy  bloom+lazy
IsVisited (No history) 0.579  6.02    0.5
IsVisited (found)      1.30   11.5    11.5
IsVisited (not found)  0.93   7.69    0.5

AddURI (new page)      95.0   2695           0
AddURI (update page)   22.5   5175           0
Typical page           304    9416    7993   1544  240

We are currently using sqlite with lazy add (the 4th column). We can get slightly better than Mork by adding the bloom filter, but this keeping it updated is complicated and will impact Ts (at least some times when we need to rebuild it)
Priority: P1 → P2
Target Milestone: Firefox 2 beta1 → Firefox 3 alpha1
Assignee: brettw → nobody
The times in comment 7 are in microseconds, not milliseconds. It does not take 9 seconds to load a page into sqlite...
Target Milestone: Firefox 3 alpha1 → Firefox 3 beta1

I'm just curious: are these times with or without IN_MEMORY_LINKS? I saw at that there were plans to keep an in-memory cache to increase performance, which is what the #define seems to do, but it seems to be disabled right now.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Target Milestone: Firefox 3 beta3 → ---
marking WONTFIX, this is about Places' first landing for Fx2.
Closed: 11 years ago
Resolution: --- → WONTFIX
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
(In reply to comment #10)
> marking WONTFIX, this is about Places' first landing for Fx2.

Dietrich, the main patch here has been landed:

So why this is marked as wontfix?
You need to log in before you can comment on or make changes to this bug.