Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments)

I noticed, while measuring memory use on the wasm demo that several MB were being waste on the excess capacity of Vectors living in the wasm::Module.  The classic C++ trick to clear capacity is copy+swap which one can write already in terms of Vector, but I'd like to avoid the copy, so I added a Vector::reallocToFit, which is only defined when IsPod<T> is true, and it uses realloc.

With these patches, I can see savings of 4.2MB (out of 14.0MB) in class(WasmModuleObject)/objects/malloc-heap/misc on http://webassembly.github.io/demo.
Attachment #8749931 - Flags: review?(n.nethercote)
Attachment #8749932 - Flags: review?(bbouvier)
Comment on attachment 8749931 [details] [diff] [review]
add-realloc-to-fit

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

I'm wondering about the name. Should it have "pod" in it to indicate the restriction? Would "resize" be better than "realloc". So... maybe "podResizeToFit"? Not sure.

::: mfbt/Vector.h
@@ +233,5 @@
> +  static inline void
> +  reallocToFit(Vector<T, N, AP>& aV)
> +  {
> +    if (aV.usingInlineStorage() || aV.mLength == aV.mCapacity)
> +      return;

Nit: MFBT style is Gecko style which braces all if-blocks.

@@ +236,5 @@
> +    if (aV.usingInlineStorage() || aV.mLength == aV.mCapacity)
> +      return;
> +    T* newbuf = aV.template pod_realloc<T>(aV.mBegin, aV.mCapacity, aV.mLength);
> +    if (MOZ_UNLIKELY(!newbuf))
> +      return;

Ditto.
Attachment #8749931 - Flags: review?(n.nethercote) → review+
Comment on attachment 8749931 [details] [diff] [review]
add-realloc-to-fit

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

::: mfbt/Vector.h
@@ +573,5 @@
>  
>    /**
> +   * Calls the AllocPolicy's pod_realloc to release excess capacity. Since
> +   * realloc is only safe on PODs, this method fails to compile if IsPod<T>
> +   * is true.

nit: "if IsPod<T> is false" or "unless IsPod<T> is true"
Comment on attachment 8749932 [details] [diff] [review]
use-realloc-to-fit

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

Nice savings!
Attachment #8749932 - Flags: review?(bbouvier) → review+
(In reply to Nicholas Nethercote [:njn] from comment #3)
Yes, that's a better name.  I had been thinking 'realloc' would imply PODness, but that's too subtle.

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
Oops, good catch!
https://hg.mozilla.org/mozilla-central/rev/29e3f72069b4
https://hg.mozilla.org/mozilla-central/rev/6f2ac4c6db27
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.