Closed Bug 1181258 Opened 4 years ago Closed 4 years ago

Limit length of URIs the seer keeps

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

Details

Attachments

(1 file, 1 obsolete file)

11.44 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
To help keep OOMs from happening in the cache, we should limit the length of URIs the seer/predictor keeps.
Attached patch patch (obsolete) — Splinter Review
Attachment #8630682 - Flags: review?(honzab.moz)
Comment on attachment 8630682 [details] [diff] [review]
patch

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

::: netwerk/base/Predictor.cpp
@@ +1442,5 @@
>      // as seen.
>      PREDICTOR_LOG(("    marking new origin entry as seen"));
> +    nsresult rv = entry->SetMetaDataElement(SEEN_META_DATA, "1");
> +    if (NS_FAILED(rv)) {
> +      PREDICTOR_LOG(("    failed to mark origin entry seen"));

is this really that fatal? (just asking)

@@ +1499,5 @@
> +  nsCString uri;
> +  nsresult rv = parsedURI->GetAsciiSpec(uri);
> +  uint32_t uriLength = uri.Length();
> +  if (ok && NS_SUCCEEDED(rv) &&
> +      uriLength > static_cast<uint32_t>(mPredictor->mMaxURILength) &&

the cast is weird, prefs can work with uints as well.

@@ +1504,5 @@
> +      uriLength > mURILength) {
> +    // Default to getting rid of URIs that are too long and were put in before
> +    // we had our limit on URI length, in order to free up some space.
> +    mKeyToDelete = key;
> +    mURILength = uriLength;

what is the mURILength for exactly?  some comments would be nice.. (I repeat myself so much..)

@@ +1508,5 @@
> +    mURILength = uriLength;
> +    mCleaningLongURI = true;
> +  }
> +
> +  if (!mCleaningLongURI && (!ok || !mKeyToDelete || lastHit < mLRUStamp)) {

would !mKeyToDelete do the same job as the new !mCleaningLongURI?

@@ +1613,5 @@
> +      entry->SetMetaDataElement("predictor::resource-count", nullptr);
> +    } else {
> +      nsAutoCString count;
> +      count.AppendInt(resourceCount);
> +      entry->SetMetaDataElement("predictor::resource-count", count.BeginReading());

when this fails (OOM probably), you should drop it to null

::: netwerk/base/Predictor.h
@@ +357,5 @@
>    uint32_t mStartupTime;
>    uint32_t mLastStartupTime;
>    int32_t mStartupCount;
>  
> +  int32_t mMaxURILength;

why is this signed?
Attachment #8630682 - Flags: review?(honzab.moz) → feedback+
(In reply to Honza Bambas (:mayhemer) from comment #3)
> is this really that fatal? (just asking)

Fatal enough to log when it happens and the predictor log module is enabled? Sure! (Also, fatal enough to stop operation, as well, since the cache won't keep that entry around without some kind of metadata - that took me a while to figure out during the first rewrite.)

> the cast is weird, prefs can work with uints as well.

Ah, never saw AddUIntVarCache, was just assuming int-only (since the old GetIntPref doesn't have a UInt counterpart). That makes me happy.

> what is the mURILength for exactly?  some comments would be nice.. (I repeat
> myself so much..)

Will add a comment (it's keeping track of the longest URI we've decided to evict so far, so we can determine if we should evict an even longer one). Actually, thinking about it more, we should maybe turn this into a list of elements to evict (instead of just one element like it was previously) so we can kill them all, with fire, in one go.

> would !mKeyToDelete do the same job as the new !mCleaningLongURI?

No, but this change will become irrelevant when I turn it into a list of elements, anyway.

> when this fails (OOM probably), you should drop it to null

Since this is updating an existing metadata element, will the old value be discarded if this fails? If not, then I don't want to drop it to null, as it still has value (keeping track of the number of other metadata elements the predictor has on that cache entry). If it does get discarded, though, there's a bunch of other cleanup that should be done if this fails.
(In reply to Nicholas Hurley [:hurley, :nwgh] from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > when this fails (OOM probably), you should drop it to null
> 
> Since this is updating an existing metadata element, will the old value be
> discarded if this fails? 

I think not.  It will fail to (re)allocate a new buffer to store the larger value.  But its failure should leave metadata intact.  Michal would know better or I would have to look into the code.

Then just leave it as it is.
Attached patch patchSplinter Review
Updated version to get rid of *all* too-large metadata entries in one go (as well as to take care of a couple other metadata entry-related issues).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff50a0989f2e
Attachment #8630682 - Attachment is obsolete: true
Attachment #8636662 - Flags: review?(honzab.moz)
Comment on attachment 8636662 [details] [diff] [review]
patch

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

::: netwerk/base/Predictor.cpp
@@ +119,5 @@
>  
> +// This is selected in concert with max-resources-per-entry to keep memory usage
> +// low-ish. The default of the combo of the two is ~50k
> +const char PREDICTOR_MAX_URI_LENGTH_PREF[] =
> +  "network.predictor.max-uri-length";

this should be in all.js

@@ +520,5 @@
>  
>  // Predictor::nsICacheEntryMetaDataVisitor
>  
>  #define SEEN_META_DATA "predictor::seen"
> +#define RESOURCE_META_DATA "predictor::resource-count"

a little bit weird name...

@@ +529,5 @@
> +{
> +  return StringBeginsWith(nsDependentCString(key),
> +                          NS_LITERAL_CSTRING(META_DATA_PREFIX)) &&
> +         !NS_LITERAL_CSTRING(SEEN_META_DATA).Equals(key) &&
> +         !NS_LITERAL_CSTRING(RESOURCE_META_DATA).Equals(key);

having META_DATA_PREFIX be "predictor::uri::" would make this code simpler..  too late, I assume.

@@ +1505,4 @@
>                                             hitCount, lastHit, flags);
>  
> +  nsCString uri;
> +  nsresult rv = parsedURI->GetAsciiSpec(uri);

hmm.. when !ok parsedURI may be null, right?

@@ +1511,5 @@
> +      uriLength > mPredictor->mMaxURILength) {
> +    // Default to getting rid of URIs that are too long and were put in before
> +    // we had our limit on URI length, in order to free up some space.
> +    nsCString nsKey;
> +    nsKey.AssignASCII(key);

I may have been asking about this before, but why exactly doesn't nsDependentCString work for you?  OTOH, mLongKeysToDelete should share the buffer and there must be a copy made anyway, so we are probably OK.  That should be checked tho.  Other option is to use dep string here.

@@ +1529,5 @@
>  Predictor::SpaceCleaner::Finalize(nsICacheEntry *entry)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  entry->SetMetaDataElement(mLRUKeyToDelete, nullptr);

only if mLRUKeyToDelete is non-null, right?
Attachment #8636662 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/6c98d3927a16
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.