Closed Bug 972652 Opened 6 years ago Closed 6 years ago

asmjscache: evict cache entries for the origin if the group or global limit is hit

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file)

Attached patch evict-on-limitSplinter Review
If a user doesn't have a bunch of free disk space the global/group quota limit will be quite low (less after bug 968272), and they may only be able to save a few asm.js cache entries before the limit is hit.  When that happens, QuotaObject::MaybeAllocateMoreSpace will fail and any further attempts to store more cache entries will fail.  This makes sense for IDB, but for a cache, you want to evict other cache entries to make room for the new one.  QuotaManager already does this at the origin level of granularity; this patch does is for cache entries within a single origin.  It's pretty easy.
Attachment #8375916 - Flags: review?(Jan.Varga)
Comment on attachment 8375916 [details] [diff] [review]
evict-on-limit

Review of attachment 8375916 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/asmjscache/AsmJSCache.cpp
@@ +142,5 @@
> +GetCacheFile(nsIFile* aDirectory, unsigned aModuleIndex, nsIFile** aCacheFile)
> +{
> +  nsCOMPtr<nsIFile> cacheFile;
> +  nsresult rv = aDirectory->Clone(getter_AddRefs(cacheFile));
> +  NS_ENSURE_SUCCESS(rv, rv);

Up to you, but it seem we started using:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
instead of NS_ENSURE_SUCCESS(rv, rv);

@@ +155,5 @@
> +}
> +
> +static nsresult
> +EvictEntries(nsIFile* aDirectory, uint64_t aNumBytes, Metadata& aMetadata,
> +             uint64_t *aFreed)

I don't like to say it, but according to the style for this module, you should change |uint64_t *aFreed| to |uint64_t* aFreed|

@@ +159,5 @@
> +             uint64_t *aFreed)
> +{
> +  uint64_t freed = 0;
> +
> +  for (int i = Metadata::kLastEntry; i >= 0 && freed < aNumBytes; i--) {

kLastEntry is declared as unsigned, other similar "for" loops use "unsigned i"

@@ +161,5 @@
> +  uint64_t freed = 0;
> +
> +  for (int i = Metadata::kLastEntry; i >= 0 && freed < aNumBytes; i--) {
> +    Metadata::Entry& entry = aMetadata.mEntries[i];
> +    unsigned moduleIndex = entry.mModuleIndex;

mModuleIndex is uint32_t, but it seems you are using "unsigned" for moduleIndex elsewhere too, maybe you should fix them all

or maybe it's "unsigned" on purpose ?

@@ +697,5 @@
>    NS_ENSURE_STATE(mQuotaObject);
>  
>    // Let the QuotaManager know we're about to consume more storage. The
> +  // QuotaManager may veto this or evict other storage. If allocation failed, it
> +  // might be because or origin is using too much space (MaybeAllocateMoreSpace

I don't understand the "it might be because or origin ..." here, is it a typo ?

@@ +706,5 @@
> +    rv = EvictEntries(mDirectory, mWriteParams.mSize, mMetadata, &freed);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    qm->DecreaseUsageForOrigin(quota::PERSISTENCE_TYPE_TEMPORARY,
> +                               mGroup, mOrigin, freed);

I would rather call DecreaseUsageForOrigin() after each file->Remove() call in EvictEntries().
Actually, it would be better to have an AutoSomething class (with a ref to freedSize) for calling DecreaseUsageForOrigin() in its destructor. So if something fails, the space allocated by successfully deleted origins will be freed too.
Attachment #8375916 - Flags: review?(Jan.Varga) → review+
(In reply to Jan Varga [:janv] from comment #1)
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }
> instead of NS_ENSURE_SUCCESS(rv, rv);

Oh right, I caught part of that discussion on platform.  I was expecting someone would either land some huge rewrite patch or, if not, we'd convert the whole of asmjscache at once.  Mixing the two forms seems kinda messy.  If you feel the time for that is now, lmk and I can write the whole-file patch as a followup.

> I don't like to say it, but according to the style for this module, you
> should change |uint64_t *aFreed| to |uint64_t* aFreed|

Sorry about these; so much muscle memory doing it the other way from SM :)

> > +  for (int i = Metadata::kLastEntry; i >= 0 && freed < aNumBytes; i--) {
> 
> kLastEntry is declared as unsigned, other similar "for" loops use "unsigned
> i"

You're right, but the reason that this for loop uses 'int' is b/c the terminating condition is i == -1.

> mModuleIndex is uint32_t, but it seems you are using "unsigned" for
> moduleIndex elsewhere too, maybe you should fix them all

The reason I used 'unsigned' is because, in SM, we tend to use 'uint32_t' to mean "the full range of uint32_t values" and 'unsigned' to mean "some small unsigned value" (and the module index is <=16).  The reason I used uint32_t for mModuleIndex is because the struct is serialized so it seemed better to explicitly size it.  However, since the serialized file format isn't expected to be at all backward-compatible (buildid is used to discard old entries), I'll just use 'unsigned' for mModuleIndex.
Summary: asmjscache: evict cache entries for the origin when if the group or global limit is hit → asmjscache: evict cache entries for the origin if the group or global limit is hit
https://hg.mozilla.org/mozilla-central/rev/3d3a0f471ca8
Assignee: nobody → luke
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.