Closed
Bug 463530
Opened 16 years ago
Closed 6 years ago
pwmgr form autofill slows down page load
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
RESOLVED
INACTIVE
People
(Reporter: taras.mozilla, Unassigned)
References
Details
(Keywords: perf)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
Comment 1•16 years ago
|
||
Can we just wrap this in a timer so that the page gets to load, and moments later the passwords fill in?
Comment 2•16 years ago
|
||
What page(s) was this measured on?
The form fill time can vary a lot, depending on:
* if the page has any forms
* if any logins are stored for the site
* number of form elements
Reporter | ||
Comment 3•16 years ago
|
||
so far my tests are on www.w3.org
I haven't filled out the forms before.
Comment 4•16 years ago
|
||
I'll feed the obvious patch through try server to see what its (albeit sketchy) Tp results have to say
Reporter | ||
Comment 5•16 years ago
|
||
I'm not a big fan of settimeouting stuff. It'll just make the browser hog cpu immediately after loading, instead of during loading. Then there is also additional settimeout overhead. It's also harded to track down cpu hogs once they are broken apart into settimeouts.
Comment 6•16 years ago
|
||
(In reply to comment #5)
> I'm not a big fan of settimeouting stuff. It'll just make the browser hog cpu
> immediately after loading, instead of during loading. Then there is also
> additional settimeout overhead. It's also harded to track down cpu hogs once
> they are broken apart into settimeouts.
No, I agree, and what you really want to do is catch this stuff *earlier* so that pages that take forever to hit DOMContentLoaded (e.g. with a script tag taking forever to pull) still get passwords inserted. Could we be smart, do it as nodes are created, but only if they have type="password", hence taking it right out of the critical path for most pageloads? Maybe dolske's already thought of all of this?
Either way, moving it to Toolkit, and cc'ng people who expressed interest.
Also, preliminary tryserver results say 1-2% Tp win, still waiting for more to trickle in.
Component: General → Password Manager
Product: Fennec → Toolkit
QA Contact: general → password.manager
Comment 7•16 years ago
|
||
Hmm - it does seem like there's opportunity for improvement here, but the second runs on try server didn't show the same win that the first run had, so I'm not confident about the magnitude of impact any more.
Reporter | ||
Comment 8•16 years ago
|
||
we should disable nsIloginmanager usage for alpha2 as it's big chunk of load time.
Comment 9•16 years ago
|
||
I wonder how much of this over head is nsLoginManager code, vs DOM code and sqlite queries. For most pages, two checks will cause us to bail out before doing much work. If there are no forms on the page, it immediately bails out. If there are forms but no logins (sql query), then it bails out.
It might be possible to optimize the last one by caching the last site checked for logins... Navigating within a site with forms but no logins would avoid this this fast path. Not clear if it would be a win, though.
Taras, how did you calculate the 138ms mentioned in comment 0? Is that the time spent in the call to fillDocument(), or something else?
Reporter | ||
Comment 10•16 years ago
|
||
Yeah, it's fillDocument. I timed the entire time it takes for the event to execute.
Comment 11•16 years ago
|
||
I just checked some timing (on my MBP)... I get times < 3 ms to get past the checks for if the page has forms, and if we have any logins. A typical page will then spend another dozen or so ms filling in logins *if* there are any stored logins for the site (which involves more DOM trudging, so it's not surprising this would be slower).
Are you measuring times on sites with or without logins available? Having logins that get filled in has a cost, but if a page has no forms or no available logins, that should be much faster.
Reporter | ||
Comment 12•16 years ago
|
||
So i checked www.w3.org and www.google.com
Neither had any stored stuff.
Comment 13•16 years ago
|
||
Could you enable signon.debug, run with the this patch, and attach the output from visiting a few sites?
Reporter | ||
Comment 14•16 years ago
|
||
Looks like it's the first load that's slowing things way down due to sql.
Comment 15•16 years ago
|
||
So, I wrote a really simple xpcshell test script that opens and empty database that ends up creating one table with one column (integer primary key). I then create a statement (SELECT * FROM sqlite_master) and I call executeStep twice. The first profile looks like this in shark:
0.0% 35.3% XUL mozStorageStatement::ExecuteStep(int*)
0.0% 35.3% libsqlite3.dylib sqlite3_step
5.9% 35.3% libsqlite3.dylib sqlite3Step
0.0% 23.5% libsqlite3.dylib sqlite3BtreeBeginTrans
0.0% 23.5% libsqlite3.dylib sqlite3BtreeGetPage
0.0% 23.5% libsqlite3.dylib sqlite3PagerAcquire
0.0% 11.8% libsqlite3.dylib sqlite3OsAccess
11.8% 11.8% libsqlite3.dylib unixAccess
0.0% 5.9% libsqlite3.dylib sqlite3OsRead
5.9% 5.9% libsqlite3.dylib unixRead
0.0% 5.9% libsqlite3.dylib pager_wait_on_lock
0.0% 5.9% libsqlite3.dylib sqlite3OsLock
5.9% 5.9% libsqlite3.dylib unixLock
0.0% 5.9% libsqlite3.dylib allocateCursor
5.9% 5.9% libsqlite3.dylib sqlite3MemSize
The second call profile looks like this:
0.0% 33.3% XUL mozStorageStatement::ExecuteStep(int*)
0.0% 33.3% libsqlite3.dylib sqlite3_step
0.0% 33.3% libsqlite3.dylib sqlite3Step
0.0% 16.7% libsqlite3.dylib sqlite3VdbeHalt
0.0% 16.7% libsqlite3.dylib qlite3BtreeCommitPhaseTwo
0.0% 16.7% libsqlite3.dylib unlockBtreeIfUnused
0.0% 16.7% libsqlite3.dylib sqlite3PagerUnref
0.0% 16.7% libsqlite3.dylib pager_unlock
0.0% 16.7% libsqlite3.dylib osUnlock
16.7% 16.7% libsqlite3.dylib unixUnlock
0.0% 16.7% libsqlite3.dylib sqlite3VdbeFreeCursor
16.7% 16.7% libsqlite3.dylib sqlite3BtreeCloseCursor
Moral of the story is that SQLite has to do some more stuff (like reading in the DB, creating the cache) when it is first accessed.
Comment 16•10 years ago
|
||
The SQLite usage is gone now so this is probably much better. Anyone want to re-measure?
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
You need to log in
before you can comment on or make changes to this bug.
Description
•