1351732 - Switch xpcom plarena usage to ArenaAllocator

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Eric Rahm [:erahm]]

We can now switch xpcom over to using ArenaAllocator instead of PLArenaPool.
It also looks like there are some |strdup|-like helpers to allocate strings
out of arenas that might be helpful to use more generally.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Add an ArenaAllocator strdup extension

This adds an extension to ArenaAllocator that provides strdup-like
functionality for various string types.

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Replace use of PLArena with ArenaAllocator in xpcom

This swaps xpcom's plarena usage to ArenaAllocator. The new ArenaStrdup
extensions are used as well.

MozReview-Commit-ID: DHDfl6IkGJL

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 2: Replace use of PLArena with ArenaAllocator in xpcom

Sorry for the churn, missed a qref after switching to the new strdup functions.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8852573 [details] [diff] [review]
Part 1: Add an ArenaAllocator strdup extension

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

Just some questions before final approval.

::: xpcom/ds/ArenaAllocatorExtensions.h
@@ +18,5 @@
> +namespace detail {
> +
> +template<typename T, size_t ArenaSize, size_t Alignment>
> +T* AllocateAndCopy(const T* aSrc, size_t aLen,
> +                   ArenaAllocator<ArenaSize, Alignment>& aArena);

Maybe just call this "DuplicateString"?

@@ +32,5 @@
> + * @return An arena allocated null-terminated string.
> + */
> +template<size_t ArenaSize, size_t Alignment>
> +char* ArenaStrdup(const char* aStr,
> +                  ArenaAllocator<ArenaSize, Alignment>& aArena)

Any particular reason these functions are:

1) Not in ArenaAllocator.h?
2) Not part of the ArenaAllocator interface?

I don't know that we need a separate header for them, but I'm certainly willing to be convinced that they can be separate from ArenaAllocator.

@@ +73,5 @@
> +template<typename T, size_t ArenaSize, size_t Alignment>
> +T* detail::AllocateAndCopy(const T* aSrc, size_t aLen,
> +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> +{
> +  const size_t byteLen = (aLen + 1) * sizeof(T);

How much should we care about overflow here?

@@ +74,5 @@
> +T* detail::AllocateAndCopy(const T* aSrc, size_t aLen,
> +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> +{
> +  const size_t byteLen = (aLen + 1) * sizeof(T);
> +  T* p = (T*)aArena.Allocate(byteLen, mozilla::fallible);

Please use static_cast<T*>.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8852606 [details] [diff] [review]
Part 2: Replace use of PLArena with ArenaAllocator in xpcom

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

\o/

Comment 6

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> Comment on attachment 8852573 [details] [diff] [review]
> Part 1: Add an ArenaAllocator strdup extension
> 
> Review of attachment 8852573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some questions before final approval.
> 
> ::: xpcom/ds/ArenaAllocatorExtensions.h
> @@ +18,5 @@
> > +namespace detail {
> > +
> > +template<typename T, size_t ArenaSize, size_t Alignment>
> > +T* AllocateAndCopy(const T* aSrc, size_t aLen,
> > +                   ArenaAllocator<ArenaSize, Alignment>& aArena);
> 
> Maybe just call this "DuplicateString"?

Sure.

> @@ +32,5 @@
> > + * @return An arena allocated null-terminated string.
> > + */
> > +template<size_t ArenaSize, size_t Alignment>
> > +char* ArenaStrdup(const char* aStr,
> > +                  ArenaAllocator<ArenaSize, Alignment>& aArena)
> 
> Any particular reason these functions are:
> 
> 1) Not in ArenaAllocator.h?
> 2) Not part of the ArenaAllocator interface?
> 
> I don't know that we need a separate header for them, but I'm certainly
> willing to be convinced that they can be separate from ArenaAllocator.

I didn't want to force nsString and friends on allocator users, but maybe that's a lost cause? The reasoning for keeping it separate was mainly that it could be a drop-in replacement for existing implementations. I'm fine folding it in, the code will be more concise.

> @@ +73,5 @@
> > +template<typename T, size_t ArenaSize, size_t Alignment>
> > +T* detail::AllocateAndCopy(const T* aSrc, size_t aLen,
> > +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> > +{
> > +  const size_t byteLen = (aLen + 1) * sizeof(T);
> 
> How much should we care about overflow here?

For nsTString's it's fine (we known they have valid byte lengths). For the raw c-string case we require that the string is null-terminated, so for that to have been allocated we know |aLen| <= max(size_t) - 1, so we at most get |aLen| * sizeof(char). Assuming sizeof(char) == 1 we're good. So I could add a static assert for that I guess.

> @@ +74,5 @@
> > +T* detail::AllocateAndCopy(const T* aSrc, size_t aLen,
> > +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> > +{
> > +  const size_t byteLen = (aLen + 1) * sizeof(T);
> > +  T* p = (T*)aArena.Allocate(byteLen, mozilla::fallible);
> 
> Please use static_cast<T*>.

Sure.

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 1: Add an ArenaAllocator strdup extension

Updated per code review. I ended up using a CheckedInt in case someone decides
to add a raw char16_t* wrapper or the like (also it was easier than writing a
discertation on why everything was okay).

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8853137 [details] [diff] [review]
Part 1: Add an ArenaAllocator strdup extension

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

::: xpcom/ds/ArenaAllocatorExtensions.h
@@ +74,5 @@
> +template<typename T, size_t ArenaSize, size_t Alignment>
> +T* detail::DuplicateString(const T* aSrc, const CheckedInt<size_t>& aLen,
> +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> +{
> +  const auto byteLen = (aLen + 1) * sizeof(T);

This doesn't look like you added CheckedInt?

Comment 9

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> Comment on attachment 8853137 [details] [diff] [review]
> Part 1: Add an ArenaAllocator strdup extension
> 
> Review of attachment 8853137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/ds/ArenaAllocatorExtensions.h
> @@ +74,5 @@
> > +template<typename T, size_t ArenaSize, size_t Alignment>
> > +T* detail::DuplicateString(const T* aSrc, const CheckedInt<size_t>& aLen,
> > +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> > +{
> > +  const auto byteLen = (aLen + 1) * sizeof(T);
> 
> This doesn't look like you added CheckedInt?

I made |aLen| a CheckedInt.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #8)
> > Comment on attachment 8853137 [details] [diff] [review]
> > Part 1: Add an ArenaAllocator strdup extension
> > 
> > Review of attachment 8853137 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: xpcom/ds/ArenaAllocatorExtensions.h
> > @@ +74,5 @@
> > > +template<typename T, size_t ArenaSize, size_t Alignment>
> > > +T* detail::DuplicateString(const T* aSrc, const CheckedInt<size_t>& aLen,
> > > +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> > > +{
> > > +  const auto byteLen = (aLen + 1) * sizeof(T);
> > 
> > This doesn't look like you added CheckedInt?
> 
> I made |aLen| a CheckedInt.

Oh.  Taking .value() won't crash on overflow in release builds, though.

Comment 11

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> (In reply to Eric Rahm [:erahm] from comment #9)
> > (In reply to Nathan Froyd [:froydnj] from comment #8)
> > > Comment on attachment 8853137 [details] [diff] [review]
> > > Part 1: Add an ArenaAllocator strdup extension
> > > 
> > > Review of attachment 8853137 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: xpcom/ds/ArenaAllocatorExtensions.h
> > > @@ +74,5 @@
> > > > +template<typename T, size_t ArenaSize, size_t Alignment>
> > > > +T* detail::DuplicateString(const T* aSrc, const CheckedInt<size_t>& aLen,
> > > > +                           ArenaAllocator<ArenaSize, Alignment>& aArena)
> > > > +{
> > > > +  const auto byteLen = (aLen + 1) * sizeof(T);
> > > 
> > > This doesn't look like you added CheckedInt?
> > 
> > I made |aLen| a CheckedInt.
> 
> Oh.  Taking .value() won't crash on overflow in release builds, though.

Well that's...unfortunate. I'll add a valid check and bail I guess.

Comment 12

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e322f24106a81673135d9e1dd839609de35688
Bug 1351732 - Part 1: Add an ArenaAllocator strdup extension. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c7254cb8293082c7caa0d9419a71eacf5f85d2
Bug 1351732 - Part 2: Replace use of PLArena with ArenaAllocator in xpcom. r=froydnj

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7e322f24106
https://hg.mozilla.org/mozilla-central/rev/a2c7254cb829