1309552 - Specify buffer size when freeing data in AllocPolicy

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

AllocPolicy has a free() method, which specifies the base pointer being freed but not its size.  This patch changes 'free_(void*)' to 'template <typename T> free_(T*, size_t numElems)' in all AllocPolicy classes and their users.

This is needed for Web Replay (bug 1207696), which (now) adds an alloc policy that needs to know the size of the buffer being freed (it ends up calling the system unmap routine, which on some platforms requires a size parameter).  It would also be nice for AllocPolicy classes which want to keep an accurate count of how much memory they have allocated but not deallocated, however.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8800221 [details] [diff] [review]
patch

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

I'd like to review the comment change, but otherwise this seems plausible.

::: mfbt/AllocPolicy.h
@@ +113,5 @@
>      return maybe_pod_realloc<T>(aPtr, aOldSize, aNewSize);
>    }
>  
> +  template <typename T>
> +  void free_(T* aPtr, size_t aNumElems)

You haven't updated the documentation comment above this class that explains the general AllocPolicy concept.  Please do so.

The docs should clarify exactly what |aNumElems| is.  Presumably it's the originally passed-in number of elements, for most allocations, but you should also explain that this value *won't* be passed in if the pointer is successfully passed to pod_realloc, in which case the new number of elements will be passed to free_.

Also -- can |aNumElems| ever *not* be one of these values?  I can imagine container classes that might want to depend on the AllocPolicy concept, that might not know this exact number -- they might only know the exact number is "not greater than" |aNumElems|.  (Similar to how malloc implementations may only know the size class of an allocation, i.e. malloc_usable_size, but not the exact number the original caller passed to malloc.)  Please make sure this is documented as well.

::: mfbt/Vector.h
@@ +143,5 @@
>      for (; src < aV.endNoCheck(); ++dst, ++src) {
>        new_(dst, Move(*src));
>      }
>      VectorImpl::destroy(aV.beginNoCheck(), aV.endNoCheck());
> +    aV.template free_<T>(aV.mBegin, aV.mCapacity);

I believe you're required to have |template| here but are *not* required to have |<T>| here, now that function template default arguments are part of C++11 (and are available in all our supported compilers per https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code "function template default arguments").

Please do a try-run with all these redundant |<T>| removed.  If it passes, land the patch that way -- otherwise what you have is fine.

Comment 2

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 3

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch

Updated/rebased patch per review comments.

Comment 5

[Jeff Walden [:Waldo]]

Can you point me at a patch that requires this API change?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Jeff Walden [:Waldo] from comment #5)
> Can you point me at a patch that requires this API change?

Bug 1464903 part 1 requires this for the AllocPolicy class it defines.  This alloc policy uses record/replay memory allocation functions that need to know the size of the region being freed.  Those memory allocation functions are defined in part 8 of the same bug, and use the size of the freed region to maintain a set of memory regions available for future allocations.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8981232 [details] [diff] [review]
patch

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

::: mfbt/AllocPolicy.h
@@ +42,5 @@
>   *  - template <typename T> T* pod_realloc(T*, size_t, size_t)
>   *      Responsible for OOM reporting when null is returned.  The old allocation
>   *      size is passed in, in addition to the new allocation size requested.
> + *  - template <typename T> void free_(T*, size_t)
> + *      The capacity passed in should match the original allocation.

s/should/must/ and s/original allocation/count of allocated elements/

::: mfbt/Vector.h
@@ +1525,5 @@
>      mLength = aLength;
>      mTail.mCapacity = kInlineCapacity;
>      Impl::moveConstruct(mBegin, aP, aP + aLength);
>      Impl::destroy(aP, aP + aLength);
> +    this->template free_(aP, aLength);

Shouldn't this be using the old value of |mTail.mCapacity|, before it was overwritten three lines up?

::: mozglue/misc/Printf.h
@@ +112,5 @@
>  struct AllocPolicyBasedFreePolicy
>  {
>    void operator()(const void* ptr) {
>      AllocPolicy policy;
> +    policy.free_(const_cast<void*>(ptr), 0);

If you can just pass 0 to some non-specifically-selected AllocPolicy on which further restrictions were clearly imposed, then the numElements passed to |free_| is only advisory.  That doesn't seem to be clearly documented in AllocPolicy.h as permissible, "should" suggesting sometimes you don't have to but not saying when.

I'm inclined to say we should require it be correct (and, of course, document it as such in AllocPolicy.h's description of free_'s semantics).  If as it appears/you suggest this is the only place that can't know the allocation count, we should just do the work to fix it.

Ultimately, tho, our AllocPolicy exposing raw memory operations (rather than letting you create Ts with invoked constructors on them) is probably a bad look, that doesn't fit C++ as the language and its object model intend/require.  Some other time/place than here, I guess.

Comment 8

[Jeff Walden [:Waldo]]

froydnj and I agreed last week on having this be freeElements instead of free_, but in light of the entire API exposing |aNumElements| controls, the only things ever allocated *are* elements.  So a rename to disambiguate seems somewhat redundant.  Just to record my change-of-position on the matter since that discussion...

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Jeff Walden [:Waldo] from comment #7)
> ::: mfbt/Vector.h
> @@ +1525,5 @@
> >      mLength = aLength;
> >      mTail.mCapacity = kInlineCapacity;
> >      Impl::moveConstruct(mBegin, aP, aP + aLength);
> >      Impl::destroy(aP, aP + aLength);
> > +    this->template free_(aP, aLength);
> 
> Shouldn't this be using the old value of |mTail.mCapacity|, before it was
> overwritten three lines up?

This call is freeing the buffer passed in by the caller, so should use the value the caller passed.  This is actually supposed to be using aCapacity, though (when I wrote the patch there was only an aLength parameter; I looked through the patch for other rebasing problems but didn't see any).

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch

I tried fixing the sprintf stuff to pass in the capacity when freeing its data, but ran into problems because sometimes that data is passed in from outside as a UniquePtr and quite a few callers would need to be fixed, e.g. the 30-odd callers of JS_sprintf_append.  Sprintf also violates the alloc policy requirements in its use of realloc, which I fixed here.  It seems simpler to make the capacity an optional argument to free_, with alloc policies allowed to require the capacity argument, so that we'd get compile errors instead of incorrect behavior if an incompatible alloc policy is used with the sprintf code.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8987653 [details] [diff] [review]
patch

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

Bah.  The std::unique_ptr interface should have been reconciled with the standard allocation policy trait when they standardized it.  :-(  I guess okay.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c8c2d8a6003
Specify buffer size when freeing data in AllocPolicy, r=waldo.

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5c8c2d8a6003