814498 - Add moz_malloc_good_size()

Status: RESOLVED WONTFIX

(Core :: Memory Allocator, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: partial patch

I want to add moz_malloc_good_size() so I can use it to right-size the chunk allocations done by nsFixedSizeAllocator.

The attached patch is a start, but omits the important bits.  I admit to being confused by the interaction of malloc_good_size(), je_malloc_good_size(), nallocm(), and je_malloc_usable_size_in_advance().

(I'd also be happy to remove je_malloc_usable_size_in_advance(), since that was the ugly name I gave the function because I didn't realize that malloc_good_size() was an established name for this kind of function.)

Comment 1

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> The attached patch is a start, but omits the important bits.  I admit to
> being confused by the interaction of malloc_good_size(),
> je_malloc_good_size(), nallocm(), and je_malloc_usable_size_in_advance().

bug 804303 part 1 makes things much clearer: only malloc_good_size it to be used, provided you #include mozmemory.h.

Comment 2

[Nicholas Nethercote [inactive]]

I'll wait on bug 804303, then!  Thanks.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

It appears we need this for bug 960873 so lets see if I can wrap it up now that bug 804303 has landed.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: moz-malloc-good-size.patch

Here is an updated version of Nicholas's patch that uses malloc_good_size() from mozmemory.h.  So far I've tested it on b2g, but not any other platforms.

https://tbpl.mozilla.org/?tree=Try&rev=be264653c19c

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8363214 [details] [diff] [review]
moz-malloc-good-size.patch

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

BTW, glandium, jemalloc3 doesn't appear to support malloc_good_size...

::: memory/mozalloc/mozalloc.cpp
@@ +230,5 @@
> +{
> +#if defined(MOZ_MEMORY)
> +    return malloc_good_size(size);
> +#else
> +    return size;

Hmm... if we can't provide the information, I think we make it obvious by returning 0, rather than providing not-quite-right information. That's what moz_malloc_usable_size() does.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 8363214 [details] [diff] [review]
> moz-malloc-good-size.patch
> 
> Review of attachment 8363214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> BTW, glandium, jemalloc3 doesn't appear to support malloc_good_size...

It does, thanks to mozjemalloc_compat.c.

> ::: memory/mozalloc/mozalloc.cpp
> @@ +230,5 @@
> > +{
> > +#if defined(MOZ_MEMORY)
> > +    return malloc_good_size(size);
> > +#else
> > +    return size;
> 
> Hmm... if we can't provide the information, I think we make it obvious by
> returning 0, rather than providing not-quite-right information. That's what
> moz_malloc_usable_size() does.

I'm not sure this makes a lot of sense. In the malloc_usable_size case, it's just impossible to know the size of the allocation pointed by the given pointer if it's not supported by the allocator. OTOH, here, if we can't know what a nice rounded size is for the allocation, we might as well just return the size we're given.

That being said, I'm reluctant to add more functions to libmozalloc. There's currently one place using malloc_good_size, why not just call malloc_good_size directly if MOZ_MEMORY is defined (if it is not, calling malloc_good_size is useless anyways)

(Note that eventually, i want to remove libmozalloc)

Comment 7

[Nicholas Nethercote [inactive]]

> That being said, I'm reluctant to add more functions to libmozalloc. There's
> currently one place using malloc_good_size, why not just call
> malloc_good_size directly if MOZ_MEMORY is defined (if it is not, calling
> malloc_good_size is useless anyways)

Sounds ok to me.

In which case this bug can be closed, and the patch in bug 960873 can just do the MOZ_MEMORY check.

Sorry for the snipe hunt, bkelly!