Closed Bug 463530 Opened 13 years ago Closed 3 years ago

pwmgr form autofill slows down page load

Categories

(Toolkit :: Password Manager, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: taras.mozilla, Unassigned)

References

Details

(Keywords: perf)

Attachments

(2 files)

Can we just wrap this in a timer so that the page gets to load, and moments later the passwords fill in?
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
so far my tests are on www.w3.org

I haven't filled out the forms before.
I'll feed the obvious patch through try server to see what its (albeit sketchy) Tp results have to say
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.
(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
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.
we should disable nsIloginmanager usage for alpha2 as it's big chunk of load time.
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?
Yeah, it's fillDocument. I timed the entire time it takes for the event to execute.
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.
So i checked www.w3.org and www.google.com

Neither had any stored stuff.
Attached patch add timing debugSplinter Review
Could you enable signon.debug, run with the this patch, and attach the output from visiting a few sites?
Attached file log
Looks like it's the first load that's slowing things way down due to sql.
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.
Blocks: 459117
The SQLite usage is gone now so this is probably much better. Anyone want to re-measure?
Depends on: 853549
Keywords: perf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.