Closed
Bug 1351732
Opened 7 years ago
Closed 7 years ago
Switch xpcom plarena usage to ArenaAllocator
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(2 files, 2 obsolete files)
23.51 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
5.48 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
This adds an extension to ArenaAllocator that provides strdup-like functionality for various string types.
Attachment #8852573 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•7 years ago
|
||
This swaps xpcom's plarena usage to ArenaAllocator. The new ArenaStrdup extensions are used as well. MozReview-Commit-ID: DHDfl6IkGJL
Attachment #8852575 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•7 years ago
|
||
Sorry for the churn, missed a qref after switching to the new strdup functions.
Attachment #8852606 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8852575 -
Attachment is obsolete: true
Attachment #8852575 -
Flags: review?(nfroyd)
Comment 4•7 years ago
|
||
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*>.
Attachment #8852573 -
Flags: review?(nfroyd) → feedback+
Comment 5•7 years ago
|
||
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/
Attachment #8852606 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Comment 7•7 years ago
|
||
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).
Attachment #8853137 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Attachment #8852573 -
Attachment is obsolete: true
Comment 8•7 years ago
|
||
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?
Attachment #8853137 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 9•7 years ago
|
||
(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•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7e322f24106 https://hg.mozilla.org/mozilla-central/rev/a2c7254cb829
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•