1423461 - More cleanups in mozalloc

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1423461 - Remove MOZALLOC_INLINE, and always use MOZ_ALWAYS_INLINE_EVEN_DEBUG.

MOZ_ALWAYS_INLINE_EVEN_DEBUG is always defined through
mozilla/Attributes.h, so the fallbacks are never used in practice. They
are just there from the old days when mozalloc.h didn't use
mozilla/Attributes.h.

Review commit: https://reviewboard.mozilla.org/r/205758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205758/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1423461 - Use noexcept in mozalloc.h on android.

Bug 1163171 removed support for building for android with GCC, and we
don't need to use throw() anymore. We can use the same code as for other
non-Windows platforms.

Review commit: https://reviewboard.mozilla.org/r/205760/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205760/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1423461 - Use mozilla/Likely.h in mozalloc.cpp.

Back when mozalloc.cpp was written, mozilla/Likely didn't exist.

Review commit: https://reviewboard.mozilla.org/r/205768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205768/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1423461 - Report the right size to the OOM handler for moz_xcalloc.

Review commit: https://reviewboard.mozilla.org/r/205770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205770/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8934846 [details]
Bug 1423461 - Report the right size to the OOM handler for moz_xcalloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205770/diff/1-2/

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8934836 [details]
Bug 1423461 - Remove MOZALLOC_INLINE, and always use MOZ_ALWAYS_INLINE_EVEN_DEBUG.

https://reviewboard.mozilla.org/r/205758/#review211360

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8934837 [details]
Bug 1423461 - Use noexcept in mozalloc.h on android.

https://reviewboard.mozilla.org/r/205760/#review211362

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8934845 [details]
Bug 1423461 - Use mozilla/Likely.h in mozalloc.cpp.

https://reviewboard.mozilla.org/r/205768/#review211364

::: commit-message-b9f84:3
(Diff revision 1)
> +Bug 1423461 - Use mozilla/Likely.h in mozalloc.cpp. r?njn
> +
> +Back when mozalloc.cpp was written, mozilla/Likely didn't exist.

Likely.h

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8934846 [details]
Bug 1423461 - Report the right size to the OOM handler for moz_xcalloc.

https://reviewboard.mozilla.org/r/205770/#review211366

::: memory/mozalloc/mozalloc.cpp:84
(Diff revision 2)
>  moz_xcalloc(size_t nmemb, size_t size)
>  {
>      void* ptr = calloc_impl(nmemb, size);
>      if (MOZ_UNLIKELY(!ptr && nmemb && size)) {
> -        mozalloc_handle_oom(size);
> +        mozilla::CheckedInt<size_t> totalSize = mozilla::CheckedInt<size_t>(nmemb) * size;
> +        mozalloc_handle_oom(totalSize.isValid() ? totalSize.value() : SIZE_MAX);

I'm trying to decide if SIZE_MAX is appropriate here. 
It doesn't feel quite right but I don't know what other value would be better.

(Relatedly, the `!ptr && nmemb && size` test suggests that if you ask for zero bytes you'll get a null pointer back, but that's not true for mozjemalloc. Maybe it's true for other allocators.)

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8934845 [details]
Bug 1423461 - Use mozilla/Likely.h in mozalloc.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205768/diff/1-2/

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8934846 [details]
Bug 1423461 - Report the right size to the OOM handler for moz_xcalloc.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205770/diff/2-3/

Comment 12

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> (Relatedly, the `!ptr && nmemb && size` test suggests that if you ask for
> zero bytes you'll get a null pointer back, but that's not true for
> mozjemalloc. Maybe it's true for other allocators.)

The code is generic for both enabled and disabled mozjemalloc, and the contract for allocation functions is that they may return null or a pointer that can be freed, so better safe than sorry.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d83c8873923
Remove MOZALLOC_INLINE, and always use MOZ_ALWAYS_INLINE_EVEN_DEBUG. r=njn
https://hg.mozilla.org/integration/autoland/rev/98b79b9db69e
Use noexcept in mozalloc.h on android. r=njn
https://hg.mozilla.org/integration/autoland/rev/e35fce376c40
Use mozilla/Likely.h in mozalloc.cpp. r=njn
https://hg.mozilla.org/integration/autoland/rev/6d34adb031e5
Report the right size to the OOM handler for moz_xcalloc. r=njn

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2d83c8873923
https://hg.mozilla.org/mozilla-central/rev/98b79b9db69e
https://hg.mozilla.org/mozilla-central/rev/e35fce376c40
https://hg.mozilla.org/mozilla-central/rev/6d34adb031e5