410307 - pwmgr isn't buffering writes when saving signons3.txt

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

While looking at Firefox file IO (ala bug 410131), I noticed that the password manager isn't buffering writes to signons3.txt. Each line in the output results in a separate write(). Originally noticed with DTrace on Solaris, also seen on OS X with ktrace.

A simple benchmark is to click "delete all" in the password manager (Preferences, Security panel). This is currently inefficient because it deletes each login 1-by-1, and the pwmgr is rewriting the whole file after each deletion. But there's something else going on here too, because if I completely disable writing to the file, it's still really slow (followup bug fodder).

With my normal signons3.txt (~80 logins, 18K):
No file IO: 1381ms (overhead)
Unbuffered: 2128ms (747ms for IO)
8K buffer:  1945ms (564ms for IO --> 24.5% faster)
 
With the signons-08.txt testcase (500 logins, 500 disabled hosts, 127K):
No file IO: 45.5s (overhead)
Unbuffered: 73.7s (28.2s for IO)
8K buffer:  64.6s (19.1s for IO --> 32.3% faster)

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 294934 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+        var safeStream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
>+                         createInstance(Ci.nsIFileOutputStream);
>+        safeStream.QueryInterface(Ci.nsISafeOutputStream);

Don't need this QI anymore, given the one below, right?

>         // WR_ONLY|CREAT|TRUNC
>+        safeStream.init(this._signonsFile, 0x02 | 0x08 | 0x20, 0600, null);
>+
>+        var outputStream = Cc["@mozilla.org/network/buffered-output-stream;1"].
>+                           createInstance(Ci.nsIBufferedOutputStream);
>+        outputStream.init(safeStream, 8192);
>+        outputStream.QueryInterface(Ci.nsISafeOutputStream); // for .finish()

Comment 2

[[e-mail]]

re that followup bug, i guess you don't really need lazy writing here (since the typical use case is adding one password and then browsing on), but maybe pwmgr could use a simple batching interface for UI operations?

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 294934 [details] [diff] [review]
Patch for review, v.1

a=mconnor on behalf of drivers

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2, checked in

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.19; previous revision: 1.18