Bug 112862 – IndexedDB: Histogram available disk space on attempt to open database

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Histogram available disk space on attempt to open database

[David Grogan]

Created attachment 194154 [details]
Patch

[David Grogan]

Created attachment 194155 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[David Grogan]

Created attachment 194156 [details]
Patch

[David Grogan]

As written, this patch requires https://code.google.com/p/chromium/issues/detail?id=222432 to land in chromium first.

[David Grogan]

Josh/Alec, could you give this a look?

[David Grogan]

Created attachment 194160 [details]
Patch

[Joshua Bell]

lgtm

[David Grogan]

WebKit API reviewers - r?

[Adam Barth]

Comment on attachment 194160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:146
> +static void HistogramFreeSpace(const char* type, String fileName)

HistogramFreeSpace -> histogramFreeSpace

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:149
> +    String name = String::format("WebCore.IndexedDB.LevelDB.Open%sFreeDiskSpace", type);

You should just use operator+ on String.  It's faster.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:156
> +    HistogramSupport::histogramCustomCounts(name.utf8().data(), clampedDiskSpaceKBytes, 1, 1e9, 11);

Should we COMPILE_ASSERT a relationship between 1e9 and INT_MAX?

[Adam Barth]

Comment on attachment 194160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review

> Source/Platform/chromium/public/Platform.h:363
> +    virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; }

Is this a blocking call?  Is there a restriction on calling this function from the main thread?

[David Grogan]

Created attachment 194372 [details]
Patch

[David Grogan]

Comment on attachment 194160 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194160&action=review

>> Source/Platform/chromium/public/Platform.h:363
>> +    virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; }
> 
> Is this a blocking call?  Is there a restriction on calling this function from the main thread?

It does block and probably shouldn't be called from the main thread. Chromium only calls it from the browser process's WebKit thread (aka IndexedDB thread). Is there a way to enforce non-main-thread usage in other ports?

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:146
>> +static void HistogramFreeSpace(const char* type, String fileName)
> 
> HistogramFreeSpace -> histogramFreeSpace

Done.

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:149
>> +    String name = String::format("WebCore.IndexedDB.LevelDB.Open%sFreeDiskSpace", type);
> 
> You should just use operator+ on String.  It's faster.

Done, but semi-awkwardly.

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:156
>> +    HistogramSupport::histogramCustomCounts(name.utf8().data(), clampedDiskSpaceKBytes, 1, 1e9, 11);
> 
> Should we COMPILE_ASSERT a relationship between 1e9 and INT_MAX?

Done, though again semi-awkwardly.

[David Grogan]

Comment on attachment 194372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194372&action=review

> Source/Platform/chromium/public/Platform.h:363
> +    virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; }

I said "other ports" forgetting that this platform layer is only available to chromium. Should I put ASSERT(!isMainThread()) somewhere or just leave as is?

[Adam Barth]

(In reply to comment #14)
> (From update of attachment 194372 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194372&action=review
> 
> > Source/Platform/chromium/public/Platform.h:363
> > +    virtual long long availableDiskSpaceInBytes(const WebString& fileName) { return 0; }
> 
> I said "other ports" forgetting that this platform layer is only available to chromium. Should I put ASSERT(!isMainThread()) somewhere or just leave as is?

We should probably leave it as-is.  I suspect we set the "main thread" bit on the in-process WebKit thread.

[Adam Barth]

Comment on attachment 194372 [details]
Patch

Thanks!

[David Grogan]

(Want to wait until the platformsupport implementation lands so that I don't get confused about unexpected zeros showing up in the histogram)

[WebKit Review Bot]

Comment on attachment 194372 [details]
Patch

Clearing flags on attachment: 194372

Committed r146560: <http://trac.webkit.org/changeset/146560>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

Reverted r146560 for reason:

invalid parameter to histogram

Committed r146561: <http://trac.webkit.org/changeset/146561>

[David Grogan]

Created attachment 194442 [details]
Patch

[David Grogan]

Adam, could I get you to re-review? The only change is on Line 152. "1, 1" became "1, 2" because the boundary is supposed to be higher than the minimum, not equal to it[1].

Untested code == broken code, indeed.

1. https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histogram.h&sq=package:chromium&type=cs&l=25

[David Grogan]

Tony, could you review this? It looks like Adam just left for vacation.

It has WebKit API in it, but abarth@ has already approved that part.

History is:
r+ from abarth
committed
rolled out
fixed non webkit-api and reuploaded
now

[Tony Chang]

Comment on attachment 194442 [details]
Patch

OK

[WebKit Review Bot]

Comment on attachment 194442 [details]
Patch

Clearing flags on attachment: 194442

Committed r146628: <http://trac.webkit.org/changeset/146628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.