1334252 - storage inspector is incredibly slow to initialize in nightly FF54

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Screenshot 2017-01-26 16.16.47.png

It seems the storage inspector is very slow to initialize in nightly.  I thought it was completely broken, but see that it finally provided some data while I was writing this bug.  See the attached screenshot for what I see for the first minute or so when opening the storage inspector.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Ben Kelly [:bkelly] from comment #0)
> Created attachment 8830890 [details]
> Screenshot 2017-01-26 16.16.47.png
> 
> It seems the storage inspector is very slow to initialize in nightly.  I
> thought it was completely broken, but see that it finally provided some data
> while I was writing this bug.  See the attached screenshot for what I see
> for the first minute or so when opening the storage inspector.

I am not sure why there is a pause although it is getting all storage types for:
  - https://disqus.com
  - https://staticxx.facebook.com
  - https://accounts.google.com
  - https://pippio.com
  - https://una.im

@Ben: I only have a 3 second wait on the page you show... is it really a whole minute for you?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I just tried again.  I guess its more like 20 to 30 seconds.  Still its very long!  It seems to be using some CPU that entire time as well, FWIW.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Just one more question... which OS version are you using?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Windows 10.  It doesn't happen in FF51 or FF52.  I haven't tested FF53 because dev-edition hasn't updated since merge day.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I can reproduce this issue.  It seems related to have a large profile, potentially with lots of IndexedDB data stored.

Here's a profile of the issue for me locally with 54:

https://clptr.io/2kt3EAI

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I've confirmed that commenting out IndexedDB storage population fixes perf issue for me.

Like :bkelly, it seems 52 is working fine, but 53 and 54 are slow.

Working on regression search now...

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Regression search indicates bug 1276339 is the cause of the issue.

Since Mike doesn't appear to have a good repro case on hand, I'll see what I can do.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It appears that main issue here is that we're iterating across all directories in the profile's storage, which for my main profile means 2,439 directories.

So far, it's not clear to me why this approach was taken, since the host name is known.  It's true that the storage directory layout does get rearranged from time to time, but we should probably try to depend on it a bit more here and avoid so much filesystem activity.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1334252 - Find IDB storage for host more directly.

After bug 1276339, storage inspector started iterating over all directories in
the profile's storage to find IDB data for a host.  This approach is too general
and can go through thousands of directories in a large profile.

By using more of what we know about the host and storage layout, we can pinpoint
the directories of interest more quickly.

Review commit: https://reviewboard.mozilla.org/r/112416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112416/

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d11d34b8d73e6d18adb0c479a2dbc50c00c0b8

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

https://reviewboard.mozilla.org/r/112416/#review114112

Thanks for the patch, great improvement! 

I just have a small doubt about the expected paths, let's see if Mike can clarify this before landing.

::: devtools/server/actors/storage.js:2062
(Diff revision 1)
>        let idbIndex = splitPath.indexOf("idb");
>        let name = splitPath[idbIndex - 1];
>        let storage = splitPath[idbIndex - 2];
>        let relative = file.substr(profileDir.length + 1);
>  
>        if (name.startsWith(sanitizedHost)) {

Maybe we can remove this check? 

Your approach guarantees that we are only getting paths that match this already.

However the startsWith() in the previous implementation makes me wonder if we can have paths that only start with `sanitizedHost` but still have some suffix appended to it ?

Comment 12

[Julian Descottes [:jdescottes]]

Mike: Can you have a look at my comment above? 

The current implementation makes me wonder if we can expect the sanitizedHost to always be the full folder name. Are there edge cases where this pattern is not true:

> profileDir/storageType/sanitizedHost/idb 

and instead we have:

> profileDir/storageType/sanitizedHost-someSuffix/idb

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

https://reviewboard.mozilla.org/r/112416/#review114118

::: devtools/server/actors/storage.js:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* globals StopIteration */

Can be removed now.

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #12)
> Mike: Can you have a look at my comment above? 
> 
> The current implementation makes me wonder if we can expect the
> sanitizedHost to always be the full folder name. Are there edge cases where
> this pattern is not true:
> 
> > profileDir/storageType/sanitizedHost/idb 
> 
> and instead we have:
> 
> > profileDir/storageType/sanitizedHost-someSuffix/idb

Let's clear the ni?

The more I look at it, the more it looks like it was a flaw in the previous implementation, which could lead to return unwanted information for a given host.

For instance let's take the test browser_storage_basic_usercontextid.js. With the old implementation, the test fails if you test first with a usercontext and then without one (because usercontext is added as a suffix to the host). But with the new implementation it works regardless of the test order.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

https://reviewboard.mozilla.org/r/112416/#review114112

> Maybe we can remove this check? 
> 
> Your approach guarantees that we are only getting paths that match this already.
> 
> However the startsWith() in the previous implementation makes me wonder if we can have paths that only start with `sanitizedHost` but still have some suffix appended to it ?

I agree it seems safe to remove, since this filtering is already happening anyway.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:bkelly, can you verify that the try build[1] also fixes the issue for you?

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d11d34b8d73e6d18adb0c479a2dbc50c00c0b8

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112416/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8f9facb486a
Find IDB storage for host more directly. r=jdescottes

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8f9facb486a

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Seems to work in my nightly profile here.  Can't remember if this is the profile that was causing me problems, though.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1276339 caused the storage inspector to take a long time to open for large profiles.
[User impact if declined]: If declined, storage inspector can take a very long time to load.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects a portion of DevTools
[String changes made/needed]: None

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8837247 [details]
Bug 1334252 - Find IDB storage for host more directly.

Fix the issue related to storage inspector. Aurora53+.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4dd9746a712