Closed Bug 451607 Opened 11 years ago Closed 8 years ago

test the performance of common Places UI actions

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: dietrich, Unassigned)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached patch wip (obsolete) — Splinter Review
from https://bugzilla.mozilla.org/show_bug.cgi?id=449640#c17:

***********
before this lands, we must be able to measure performance of various actions
that flex this code. a low-bar approach i've been thinking about is to write
chrome mochitests that contain perf scores in the test comments.

for example, we could write a mochitest that times the opening of the history
menu, and the test comment could be "ui-perf|places|open history menu|20ms" or
something like that.

eventually we could build a datastore and UI, making it generally useful, but
the log data alone provides a simple solution for our immediate needs.
***********

the attached patch provides a basic test framework for doing this, and tests the history and bookmarks sidebars. there's no reporting at all yet - need to manually look at the logs before/after your check-in.

issues:
- should up the history visits to 10,000
- or maybe should do multiple runs: empty profile, small profile, large profile
- i'm not able to get the menus to open yet, so that test is disabled

other tests that should be added:
- open the library
- add different quantities of visits, bookmarks
- remove different quantities of visits, bookmarks
- annotations
- switch between the various history sidebar views
this blocks landing of the partitioning code. we need to get baseline data before landing.
Blocks: 450290
Attached patch basic test set (obsolete) — Splinter Review
basic framework and some tests to start with. we should get this in asap, to start building baseline data for these tests, to determine if the numbers are stable enough to be useful.

can run tests manually like so:

$ cd ~/path/to/objdir/_tests/testing/mochitest
$ python runtests.py --browser-chrome --test-path=../browser/browser/components/places/tests/perf/

in tinderbox logs, you can find test results by searching for "ui-perf-test".
Attachment #334935 - Attachment is obsolete: true
Attachment #337122 - Flags: review?(mak77)
You know that these won't run by default because they aren't prefixed with test_ right?

I do think we should get some automated system to add perf tests though.

Also, I think it'd be best to have each one of these in it's one file so if we ever regress things, it's easy to profile (and shark hooks like I did in the other file would be nice, IMO).
(In reply to comment #3)
> You know that these won't run by default because they aren't prefixed with
> test_ right?

these are browser chrome tests, so needs be prefixed with browser_. see:

http://developer.mozilla.org/en/Browser_chrome_tests

> 
> I do think we should get some automated system to add perf tests though.
> 
> Also, I think it'd be best to have each one of these in it's one file so if we
> ever regress things, it's easy to profile (and shark hooks like I did in the
> other file would be nice, IMO).

agreed, will break em out.
oh...the existing test in that path is a chrome test, so I was making the assumption that these were as well.
Attachment #337122 - Flags: review?(mak77)
Comment on attachment 337122 [details] [diff] [review]
basic test set

could you add a test that opens the awesomebar too? that would be interesting

it would be interesting also changing history sidebar view by, clearly this is a first step so you don't have to put all tests now :)

also for checkin i would cleanup the latest part with commented out, commenting on why it's commented out

+        var bookmarks_total = total_visits/10; // bookmark a tenth of your visits
+        var bookmarks_per_day = bookmarks_total/days;
+
+        // reset visit date counter
+        visit_date_microsec = Date.now() * 1000;

if you reset visit_date_microsec you're not bookmarking your visits, rather you're adding new bookmarks on new pages, so the comment "// bookmark a tenth of your visits" is not completely correct

+ make_test_report("open_history_sidebar", duration)

what about adding a unit, like duration+"ms"

also i get  TEST-UNEXPECTED-FAIL because of empty tests...

Maybe you shoul repeat the test 10 times, discard worst and best and then take an average time?

How can we track these results for future?

good starting point!
> also i get  TEST-UNEXPECTED-FAIL because of empty tests...

which empty tests? i do not see this.

> 
> Maybe you shoul repeat the test 10 times, discard worst and best and then take
> an average time?

sounds good, will do.

> 
> How can we track these results for future?

future bug
(In reply to comment #7)
> > also i get  TEST-UNEXPECTED-FAIL because of empty tests...
> 
> which empty tests? i do not see this.

this

+// test opening of the library
+ptests.push({
+  run: function() {
+  }
+});
Attached patch moreSplinter Review
changes:
- broke tests out into separate files
- added report param for specifying units
- added tests for the different history sidebar views
- run UI tests 10x times, report avg sans highest/lowest

issues for followup:
- menu test is still disabled
- shared header
Attachment #337122 - Attachment is obsolete: true
Attachment #337605 - Flags: review?(mak77)
Attached patch unbitrot (obsolete) — Splinter Review
sorry for late (damn late!), i've unbitrotted this for you, so forgive me :)

+
+/**
+ *
+ * Update Behavior in Firefox:
+ *
+ * Five seconds after the browser UI starts up, the livemark service's update
+ * timer is started via nsILivemarkService.start(). The update timer fires
+ * immediately and by default every 15 minutes (is dynamically calculated as
+ * gExpiration/4), with a maximum limit of 1 hour.
+ *
+ * When the update timer fires, it iterates over the list of livemarks, and will
+ * refresh a livemark *only* if it's expired.
+ *
+ * The expiration time for a livemark is determined by using information
+ * provided by the server when the feed was requested, specifically
+ * nsICacheEntryInfo.expirationTime. If no information was provided by the 
+ * server, the default expiration time is 1 hour.
+ *
+ * Users can modify the default expiration time via the 
+ * browser.bookmarks.livemark_refresh_seconds preference.
+ *
+ */

Since we are about changing this i'd remove the description for now

fixe lines over 80 chars where possible
check for exceptions when instantiating services

apart that, r=me
Attachment #337605 - Attachment is obsolete: true
Attachment #337673 - Flags: review+
Attachment #337605 - Flags: review?(mak77)
Comment on attachment 337673 [details] [diff] [review]
unbitrot

WTF, I posted on wrong bug :\
Attachment #337673 - Attachment is obsolete: true
Attachment #337673 - Flags: review+
Attachment #337605 - Attachment is obsolete: false
Attachment #337605 - Flags: review?(mak77)
Comment on attachment 337605 [details] [diff] [review]
more

+        var bookmarks_total = total_visits/10; // bookmark a tenth of your URLs

still not clear, these are NEW urls, not our urls :) however not a problem

+const TEST_COUNT = 10;

what about TEST_REPEAT_COUNT?

+    // XXX does not work, is still open=false immediately after setting it to true
+    menu.open = true;

does it change something using menu.lastChild.showPopup()?

+/*
+Tests the performance of various UI actions.
+
+setup:
+- add XXX visits distributed over XXX days
+- add XXX bookmarks distributed over XXX days
+
+tests done:
+- add many visits
+- add many bookmarks 
+- open the default history sidebar view
+- open the bookmarks sidebar
+
+tests todo:
+- open the history menu
+- open the bookmarks menu
+- open the other history sidebar views
+- open the library
+- delete many bookmarks
+- clear history
+- annotations?
+- autocomplete?
+
+*/

this is in history sidebar, should not talk about other tests


Is it complex adding a test that opens and close the awesomebar?
Attachment #337605 - Flags: review?(mak77) → review+
Jonas, Ben T, and I were talking about performance tests in mochitest today (regarding an unrelated matter).  How do you plan to address the problem that the tests may bog down on a unit test machine (those machines are flaky with their run times) and cause spurious oranges on tboxen?

Maybe we can use what you've got for other performance related mochitests.
(In reply to comment #13)
> Jonas, Ben T, and I were talking about performance tests in mochitest today
> (regarding an unrelated matter).  How do you plan to address the problem that
> the tests may bog down on a unit test machine (those machines are flaky with
> their run times) and cause spurious oranges on tboxen?

don't know. i'm going to check these in, and watch the numbers to see if they're at all reliable. if it's too noisy, i'll get a dedicated set of boxes to run only these tests. or something.
Attached patch for checkinSplinter Review
tree is closed. will check-in when it re-opens.
Dietrich: Let me know how well the numbers report.  I might want to use this approach for other types of testing too.
A windows user has reported doing the "Cut" action on a folder that had over 3000 bookmarks in multiple subfolders was taking over half an hour, with process monitor showing millions of accesses to places.sqlite / places.sqlite-journal
<Megzlna> the file offset its reading in process monitor is changing very slowly it reads the entire 12MB then does it over and over again every time rereading the whole file and the last offset it reads is slowly going down.
<Megzlna> judging by watching process monitor, it appears that the whole entire 12MB is being re-read for every single entry in the bookmarks
<Megzlna> so like, reread 12MB 3000 times
<Megzlna> it got all the way down to 0MB finally
<Megzlna> and then went back to 8MB
<Megzlna> it finished, so about 1:15 total
<Megzlna> And all my data is gone
<Megzlna> the folder is gone, and there's no "Paste"
<Megzlna> possibly because I cut and pasted something in the past hour
(expletives removed)
(In reply to comment #18)
> A windows user has reported doing the "Cut" action on a folder that had over
> 3000 bookmarks in multiple subfolders was taking over half an hour, with
> process monitor showing millions of accesses to places.sqlite /
> places.sqlite-journal

which version of the browser?
Dietrich, can we resolve this and file a new bug about using the provided data in some sort of graph/tabular view?
Depends on: 523146
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.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Not working on this anymore, so unassigning.
Assignee: dietrich → nobody
the purpose is still good, but the method is not, we also removed these tests from the tree. telemetry is better.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Yeah, this bug is dead, thanks for coming back around and closing it.

Telemetry is good, but not for per-check-in regression testing. Any new perf tests should use peptest once it's up and running on tbpl: https://wiki.mozilla.org/Auto-tools/Projects/peptest.
You need to log in before you can comment on or make changes to this bug.