Closed Bug 1207519 Opened 4 years ago Closed 4 years ago

HashTable shrink ignores allocation failures that may have been reported


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: jonco, Assigned: jonco)




(1 file, 2 obsolete files)

HashTable::checkUnderloaded() and compactIfUnderloaded() call changeTableSize() to shrink the table and they ignore any allocation failure, leaving the table larger than necessary in that case.

However, depending on the AllocPolicy, allocation failure may have side effects.  For example TempAllocPolicy (the default) reports the failure to the JSContext.  Obviously this is wrong.

These calls are made from methods that don't return an error (e.g. the remove operation and the Enum destructor), so propagating the error is not easy.

Alternatively we could add an alloc policy method to reverse any side effect of the failed allocation.  For TempAllocPolicy this would mean calling recoverFromOutOfMemory() on the context.
Attached patch fix-hashtable-remove (obsolete) — Splinter Review
Patch to add ignoreAllocFailure() method to AllocPolicy and use this in the HashTable implementation when we ignore failure.
Attachment #8664760 - Flags: review?(jwalden+bmo)
Blocks: 1186982
Comment on attachment 8664760 [details] [diff] [review]

Review of attachment 8664760 [details] [diff] [review]:

::: mfbt/AllocPolicy.h
@@ +35,5 @@
>   *      size is passed in, in addition to the new allocation size requested.
>   *  - void free_(void*)
> + *  - void ignoreAllocFailure()
> + *      Can be called by client code to clear any OOM condition reported after
> + *      one of the allocation methods failed.

This only seems to work, *if* reporting OOM is reversible.  It is for the JS engine allocation policy, but it might not be generally.  We could imagine failure-reporting triggering a printf, for example, that's irreversible.

It seems to me that really, what's desired is allocation operations that do *not* report on OOM (or on allocation overflow, for that matter) but simply return nullptr or do nothing.  I'd analogize this to Gecko's handling of OOM: the default operations just crash, but you can select variants that return nullptr without crashing if you need them (mostly for large allocations, is our conceit).

The multiplicity of allocation operations seems somewhat unideal, but I guess them's the shakes as long as we're going to (reasonably) conflate reporting with attempting and failing.
Attachment #8664760 - Flags: review?(jwalden+bmo)
Attached patch fix-hashtable-remove v2 (obsolete) — Splinter Review
Ok, here's an updated patch.  This adds maybe_pod_blah() methods which don't report allocation failure.
Attachment #8664760 - Attachment is obsolete: true
Attachment #8665534 - Flags: review?(jwalden+bmo)
It turns out MSVC 2013 doesn't like this patch.  I get the following error on Windows compiles:

c:\src\inbound\js\src\debug-build\dist\include\js\hashtable.h(1134) : fatal error C1001: An internal error has occurred in the compiler.
(compiler file 'f:\dd\vctools\compiler\utc\src\p2\main.c', line 228)
 To work around this problem, try simplifying or changing the program near the locations listed above.
It seems MSVC really doesn't like template methods in template class.  Here's a formulation that does work using an extra (non-template) parameter.
Attachment #8665534 - Attachment is obsolete: true
Attachment #8665534 - Flags: review?(jwalden+bmo)
Attachment #8666778 - Flags: review?(jwalden+bmo)
Comment on attachment 8666778 [details] [diff] [review]

Review of attachment 8666778 [details] [diff] [review]:

::: js/public/HashTable.h
@@ +1123,5 @@
>              keyHash -= (sRemovedKey + 1);
>          return keyHash & ~sCollisionBit;
>      }
> +    enum FailureBehavior { DontReportFailure = 0, ReportFailure };

Minor preference for = false, = true on these.

::: js/src/vm/Runtime.h
@@ +2092,2 @@
>      T* pod_malloc(size_t numElems) {
> +        return maybe_pod_malloc<T>(numElems);

I think it's better to be consistent and make these return

  return runtime->pod_malloc<T>(numElems);

and so on for pod_calloc/pod_realloc, not assume pod_malloc's behavior is the same as maybe_pod_malloc's behavior.
Attachment #8666778 - Flags: review?(jwalden+bmo) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1210607
Duplicate of this bug: 666631
You need to log in before you can comment on or make changes to this bug.