1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P3)

Description

[JW Wang [:jwwang] [:jw_wang]]

Since bug 969114 moves mPartialBlockBuffer to a separate heap allocation, we don't need int64_t for proper alignment (the memory return by malloc() is always properly aligned).

Changing the type to UniquePtr<uint8_t[]> also saves some reinterpret_casts.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>.

Since bug 969114 moves mPartialBlockBuffer to a separate heap allocation,
we don't need int64_t for proper alignment for the memory returned by
malloc() is always properly aligned.

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Looks like Gerald is on vacation.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8857739 [details]
Bug 1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>.

https://reviewboard.mozilla.org/r/129698/#review132832

::: commit-message-efa44:4
(Diff revision 1)
> +Bug 1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>.
> +
> +Since bug 969114 moves mPartialBlockBuffer to a separate heap allocation,
> +we don't need int64_t for proper alignment for the memory returned by

What do you mean by alignment, memory alignement? If so that is not correct.

jemalloc do not align data.

AFAIK, only malloc on mac returns an aligned buffer

If your changes rely on malloc returning a memory aligned block, then it wont work

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard-hg.mozilla.org/gecko/rev/329adb8a4414f32e2f83ba79af06d288d7b25705#l2.16

MakeUnique<> invokes the new operator which returns a memory block which is at least int64_t aligned. Is that correct?

Comment 5

[Jean-Yves Avenard [:jya]]

I'm certain it is not aligned. It's platform specific, and more often than not, jemalloc does not align memory.

Hence why we had to have things like AlignedBuffer.

This was confirmed by :njn

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Nah, AlignedBuffer is used for higher alignment requirement that is not provided by new/malloc like 32byte or 64byte alignment.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8857739 [details]
Bug 1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>.

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

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8857739 [details]
Bug 1355406 - Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>.

https://reviewboard.mozilla.org/r/129698/#review132940

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3985a21e25a3
Change the type of MediaCacheStream::mPartialBlockBuffer to UniquePtr<uint8_t[]>. r=jya

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3985a21e25a3