Test Build: Asynchronous location bar

[sdwilsh]

Hey all,

A few months ago I decided to try to use the asynchronous storage API that was added in Firefox 3.1 to help reduce the pain of disk IO on the main thread. Sadly, it became quite apparent that this was going to be too big of a change and need to much work to make it into 3.1, so I put off doing any more work on it. However, this week I started working on the patch again, updating it to work with the changes to the location bar and the storage back-end. Today I finally got it passing all of our existing tests (although, I know of at least one condition where it fails and is untested).

Now that it's passing all tests, I feel comfortable posting a test build for folks to try and see if it helps or not. I should note that the current implementation is pretty dumb and doesn't take many opportunities speed up results. Additionally, there are some other performance wins that are on my mind that become a lot easier to do with this newer implementation.

Admittedly, I haven't benchmarked this yet, so I don't know how it compares to the existing code. During causal use, however, it feels no slower than the existing implementation, but I don't usually have issues with it. The goal here is to help out those who do have performance issues with the location bar. In fact, that's exactly the feedback I'm looking to get. So, if you are feeling ambitious and willing to live on the wild side for a bit, I'd like you try this test build. After a little bit of use, let me know if you think the results are faster, slower, or about the same. Note: this is build off of mozilla-central, so it's like a 3.2a1pre build.

Your feedback is greatly appreciated!

[sysKin]

Thanks for the test build, using it now. I never had a performance problem with locationbar* but all speed improvements are good.

Can you point us to the bug# which this fixes? Also, can you say what kind of storage operations are now moved off the main thread - populating the awesomebar, updating history after visiting a site, both, something else?

* hard limit history of 15 days does wonders for speed; imho bug 425219 should be just fixed - firefox is sacrificing a lot of speed for a fuzzy idea of "randomly retaining stuff even when not needed or expected, under a condition that retained stuff can be deleted at random" and it should just stop doing that (imho of course).

[mak77]

hard limit history of 15 days does wonders for speed; imho bug 425219 should be just fixed - firefox is sacrificing a lot of speed for a fuzzy idea of "randomly retaining stuff even when not needed or expected, under a condition that retained stuff can be deleted at random" and it should just stop doing that (imho of course).

I think expiration simplification is one of the targets for 3.2, still we need a finalized plan for that. That said, i don't think hard limiting history will be a perf win as we expect it to be, our queries are not so slow, often the code the manages results can be slower than the query itself. We have people running with 999 days of history with very limited perf problems, that we are trying to solve, so hard limiting to a small value (15 days for example) will bring no real benefit, but cause loss of functionality. If people increases the history expiration limits is because they need more data, not less.
Personally i use a lot the awesomebar with bugzilla entries, and i can find bugs losing the minor time possible, and that's because i've visited those bugs, many many days ago.

Btw, notice we don't retain random stuff, nor delete it randomly

[sysKin]

Hey sdwilsh I just had a deadlock.
I was browsing a random page and clicked on a link while a page was still loading, and browser froze. CPU usage was zero. I tried activating a debugger and all threads were idling in ntdll.dll.

Of course I have no way of knowing if your patch is responsible, I didn't even have symbols (are there symbols for tryserver builds?) but I haven't seen such freeze since forever.

That said, i don't think hard limiting history will be a perf win as we expect it to be

Oh I don't advocate limiting the history below its current 90 days. It's fine. I do, however, advocate not keeping it *above* 90 days, particularly without user's knowledge and control.

Notice that current situation is a useless-UI problem: the history setting available under options is a "what is guaranteed" setting only (not "what browser does" setting), and for vast majority of users (who don't hit the 40k limit) changing it is a no-op. Particularly lowering it.

our queries are not so slow, often the code the manages results can be slower than the query itself.

Sure, more data is more work in general.
Size of the places.sqlite file mostly has effect on roaming (networked) profiles. Still, you can't make database contain twice as many items (180 days actual expiration vs 90 days as per user setting) and expect it to have no effect

Btw, notice we don't retain random stuff, nor delete it randomly

Well, very few things in computing world is random what I meant by "randomly" is "in a way user can't predict".

[chob]

With the tryserver build and my 50MB places.sqlite file with 1yr of history I can't say I noticed a difference in speed one way or the other on my ancient AthlonXP2400.

[sdwilsh]

Can you point us to the bug# which this fixes? Also, can you say what kind of storage operations are now moved off the main thread - populating the awesomebar, updating history after visiting a site, both, something else?

bug 455555

I'll probably blog about all the changes we've done with places on threading for 3.1, but I'll keep it out of here for now to try and keep this thread on topic.

This particular change doesn't introduce a whole lot of threading (this is partly why it's a dumb implementation). It only reads data from the disk asynchronously, but the filtering is still done on the main thread. The next step is to evaluate different ways of filtering off of the main thread and see how that impacts performance.

[sdwilsh]

Hey sdwilsh I just had a deadlock.
I was browsing a random page and clicked on a link while a page was still loading, and browser froze. CPU usage was zero. I tried activating a debugger and all threads were idling in ntdll.dll.

Of course I have no way of knowing if your patch is responsible, I didn't even have symbols (are there symbols for tryserver builds?) but I haven't seen such freeze since forever.

Deadlock and not a crash, right? Sadly, try server builds do not have symbols, so can't really figured it out. I've had at least two reports of a crash though, but those look like they are the result of the image cache, and shouldn't have anything to do with my changes.

[sdwilsh]

With the tryserver build and my 50MB places.sqlite file with 1yr of history I can't say I noticed a difference in speed one way or the other on my ancient AthlonXP2400.

With this initial implementation, that's exactly what I am looking for. This is just the first step to make the code easier to modify.

[sdwilsh]

New build can be found here (updated first post as well):
https://build.mozilla.org/tryserver-bui ... 328a1c0aa/

[Mardak]

For those who want to compare speeds between trunk and these async builds, copy/paste the following code into Error Console. It'll open a new page that looks like the chunk of text after the code block..

Code: Select all

/* Don't repeat queries... */
var queries = ["m", "mo", "moz", "mozi", "mozil", "mozill", "mozilla", "f", "fo", "for", "foru", "forum", "forums"];
var repeat = 5;

const Cc = Components.classes;
const Ci = Components.interfaces;

var buf = [];
function D(m) {
  /* Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(m); */
  if (m !== null)
    buf.push(m);
  else
    open("data:text/html," + buf.join("<br/>"));
}

function start() {
  /* Keep testing until we have enough results */
  if (repeat--)
    test(queries.slice());
  /* Process results for min, avg, max */
  else {
    for each (var q in queries) {
      var out = ["Query:", q];
      var r = R[q];
      for (var i = 0; i < 2; i++) {
        var max = -Infinity, min = Infinity, sum = 0;
        for (var j = r.length; --j >= 0; ) {
          var d = r[j][i];
          max = Math.max(max, d);
          min = Math.min(min, d);
          sum += d;
        }
        out = out.concat([i == 0 ? "First:" : "Finish:", [min, Math.round(sum / r.length), max]]);
      }
      D(out.join(" "));
    }
    D(null);
  }
}

function test(Q) {
  /* Grab the next query or start over if we're out */
  var q = Q.shift();
  if (q === undefined) {
    start();
    return;
  }

  /* Track various times */
  var startTime = Date.now();
  var fDelay = null;

  Cc["@mozilla.org/autocomplete/search;1?name=history"].getService(Ci.nsIAutoCompleteSearch).startSearch(q, "", null, ({
    onSearchResult: function(a, aResult) {
      var delay = Date.now() - startTime;
      switch (aResult.searchResult) {
        case aResult.RESULT_SUCCESS_ONGOING:
          if (fDelay === null)
            fDelay = delay;
          break;
        case aResult.RESULT_NOMATCH:
        case aResult.RESULT_SUCCESS:
          R[q].push([fDelay === null ? delay : fDelay, delay]);
          /* D([q, R[q]]); */
          setTimeout(test, 0, Q);
          break;
      }
    }
  }));
}

/* Initialize results array */
var R = {};
for each (var q in queries)
  R[q] = [];
start();

Code: Select all

Query: m First: 37,38,39 Finish: 37,38,39
Query: mo First: 7,7,8 Finish: 140,145,161
Query: moz First: 7,7,8 Finish: 140,147,150
Query: mozi First: 7,9,11 Finish: 7,9,11
Query: mozil First: 7,7,8 Finish: 7,7,8
Query: mozill First: 7,8,9 Finish: 7,8,9
Query: mozilla First: 7,8,9 Finish: 7,8,9
Query: f First: 37,37,39 Finish: 37,37,39
Query: fo First: 7,8,8 Finish: 144,145,145
Query: for First: 7,8,8 Finish: 143,144,145
Query: foru First: 7,8,9 Finish: 145,147,149
Query: forum First: 7,8,8 Finish: 7,8,8
Query: forums First: 7,7,8 Finish: 420,422,424

It shows the minimum, average, maximum of 5 times to get the first result and the finish time for a given query.

The numbers aren't useful by themselves, but if you pair them with the numbers you get for Firefox 3, Firefox 3.1, trunk, async builds, that'll be interesting data.

Like the above data is trunk vs the following is Firefox 3

Code: Select all

Query: m First: 10,14,27 Finish: 10,14,27
Query: mo First: 10,11,12 Finish: 101,116,122
Query: moz First: 10,10,11 Finish: 119,123,136
Query: mozi First: 11,13,22 Finish: 11,13,22
Query: mozil First: 10,12,15 Finish: 10,12,15
Query: mozill First: 10,11,12 Finish: 10,11,12
Query: mozilla First: 10,12,16 Finish: 10,12,16
Query: f First: 10,10,10 Finish: 10,10,10
Query: fo First: 10,11,12 Finish: 118,120,121
Query: for First: 10,16,40 Finish: 121,127,149
Query: foru First: 10,11,11 Finish: 125,125,125
Query: forum First: 10,10,11 Finish: 10,10,11
Query: forums First: 10,11,11 Finish: 373,414,569

(The async builds don't work for me...)

[sdwilsh]

I should note that I'm getting reports that the async builds don't work for some people. I'm not sure what's up with that, but I hope to look into it next week.

[sdwilsh]

New build out here:
https://build.mozilla.org/tryserver-bui ... 58fef1cb9/

This runs for me (I hadn't tried try-server builds in the past but my local build worked). Hopefully this works out better for folks who were having issues. Minor fixes to the algorithm for correctness.

[sysKin]

New build out here:
https://build.mozilla.org/tryserver-bui ... 58fef1cb9/

Hi, for me this build doesn't show any urlbar autocomplete popup at all. Also in safe mode. No errors in error console.

[sdwilsh]

I'll try a windows build at the office on Monday. Sadly, I can't debug this from home right now.

[a;skdjfajf;ak]

This just got checked into Trunks..
https://bugzilla.mozilla.org/show_bug.cgi?id=478097