Bug 165793 – [GStreamer] use FastMalloc-based GstAllocator

• Component: Platform
• Status: RESOLVED FIXED

[Zan Dobersek]

With large amounts of memory being allocated via GStreamer, it should be interesting to see what improvements could be achieved by diverting those allocations to bmalloc.

[Zan Dobersek]

Created attachment 296994 [details]
WIP

[Xabier Rodríguez Calvar]

Comment on attachment 296994 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=296994&action=review

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
> +        fprintf(stderr, "initializeGStreamer(): using FastMalloc allocator %p\n", allocator);

I would GST_INFO this, though it would require to add a debug category for this.

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:36
> +    auto* mem = reinterpret_cast<GstMemoryFastMalloc*>(fastAlignedMalloc(alignment + 1, headerSize + allocationSize));

I prefer typed things when they are non-trivial, which IMHO is  the case.

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:40
> +    mem->data = reinterpret_cast<guint8*>(mem) + headerSize;

uint8_t

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:53
> +static void gst_allocator_fast_malloc_free(GstAllocator*, GstMemory* mem)

I would ASSERT here if the allocator is right.

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:82
> +    auto* copy = reinterpret_cast<GstMemoryFastMalloc*>(fastAlignedMalloc(alignment + 1, headerSize + allocationSize));

Proper typing?

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:89
> +    copy->data = reinterpret_cast<guint8*>(copy) + headerSize;

uint8_t

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:104
> +    auto* sharedMem = reinterpret_cast<GstMemoryFastMalloc*>(fastMalloc(sizeof(GstMemoryFastMalloc)));

Proper typing?

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:118
> +        auto* parent = reinterpret_cast<GstMemoryFastMalloc*>(mem1->base.parent);

Proper typing?

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:135
> +    auto* gobjectClass = G_OBJECT_CLASS(klass);
> +    gobjectClass->finalize = gst_allocator_fast_malloc_finalize;
> +
> +    auto* gstAllocatorClass = GST_ALLOCATOR_CLASS(klass);

Proper typing?

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.h:29
> +    guint8* data;

uint8_t

[Zan Dobersek]

Comment on attachment 296994 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=296994&action=review

>> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
>> +        fprintf(stderr, "initializeGStreamer(): using FastMalloc allocator %p\n", allocator);
> 
> I would GST_INFO this, though it would require to add a debug category for this.

It's a debugging leftover, can be removed.

>> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:36
>> +    auto* mem = reinterpret_cast<GstMemoryFastMalloc*>(fastAlignedMalloc(alignment + 1, headerSize + allocationSize));
> 
> I prefer typed things when they are non-trivial, which IMHO is  the case.

This is typed like anything else, but it removes the duplicated information.

Also, static_cast<> can be used.

[Zan Dobersek]

Comment on attachment 296994 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=296994&action=review

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> +        gst_allocator_set_default(allocator);

This could be moved before gst_init(), maybe? Seems to work (tm).

[Zan Dobersek]

Created attachment 297004 [details]
WIP

[Enrique Ocaña]

Comment on attachment 297004 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=297004&action=review

> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:28
> +#define WEBKIT_IS_VIDEO_SINK(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_VIDEO_SINK))

This seems a leftover from the original code you used as template. Maybe you'd like it to be GST_IS_ALLOCATOR_FAST_MALLOC(), or maybe you're like to remove it if it's not used anywhere.

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> >> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
> >> +        fprintf(stderr, "initializeGStreamer(): using FastMalloc allocator %p\n", allocator);
> > 
> > I would GST_INFO this, though it would require to add a debug category for this.
> 
> It's a debugging leftover, can be removed.

I think it is useful information to see in a GStreamer log.

Which brings another question to me: if we change the default allocator, we would be affecting other GStreamer multimedia playback outside WebKit. I'm beginning to wonder about the consequences of this.

> >> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:36
> >> +    auto* mem = reinterpret_cast<GstMemoryFastMalloc*>(fastAlignedMalloc(alignment + 1, headerSize + allocationSize));
> > 
> > I prefer typed things when they are non-trivial, which IMHO is  the case.
> 
> This is typed like anything else, but it removes the duplicated information.

I know it is typed and I still prefer to see the explicit types at the code.


(In reply to comment #4)
> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> > +        gst_allocator_set_default(allocator);
> 
> This could be moved before gst_init(), maybe? Seems to work (tm).

I wouldn't do it.

[Zan Dobersek]

(In reply to comment #7)
> Which brings another question to me: if we change the default allocator, we
> would be affecting other GStreamer multimedia playback outside WebKit. I'm
> beginning to wonder about the consequences of this.
> 

Yeah, this could be a bit of a blocker.

Also, Phil commented that our allocator should use the webkit_ prefix.

[Zan Dobersek]

Turns out it's quite hard to get a GstAllocator implementation right. Instead, let's use FastMalloc in specific places as required, e.g. what bug #167928 does for MediaSourceClientGStreamerMSE.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #4)
> Comment on attachment 296994 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296994&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> > +        gst_allocator_set_default(allocator);
> 
> This could be moved before gst_init(), maybe? Seems to work (tm).

No, it can't. gst_init calls _priv_gst_allocator_initialize() that initializes the allocators hash table and set the system malloc as the default one. So, any custom allocator must be set after gst_init().

[Carlos Garcia Campos]

(In reply to Xabier Rodríguez Calvar from comment #7)
> (In reply to comment #3)
> > >> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:161
> > >> +        fprintf(stderr, "initializeGStreamer(): using FastMalloc allocator %p\n", allocator);
> > > 
> > > I would GST_INFO this, though it would require to add a debug category for this.
> > 
> > It's a debugging leftover, can be removed.
> 
> I think it is useful information to see in a GStreamer log.
> 
> Which brings another question to me: if we change the default allocator, we
> would be affecting other GStreamer multimedia playback outside WebKit. I'm
> beginning to wonder about the consequences of this.

No, this would only affect gst used by the web process. I think we could move the gst initialization to the web process main and do it unconditionally. At least here it's quite fast.

DBG: gst initialized in 0,012539

> > >> Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:36
> > >> +    auto* mem = reinterpret_cast<GstMemoryFastMalloc*>(fastAlignedMalloc(alignment + 1, headerSize + allocationSize));
> > > 
> > > I prefer typed things when they are non-trivial, which IMHO is  the case.
> > 
> > This is typed like anything else, but it removes the duplicated information.
> 
> I know it is typed and I still prefer to see the explicit types at the code.
> 
> 
> (In reply to comment #4)
> > > Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.cpp:162
> > > +        gst_allocator_set_default(allocator);
> > 
> > This could be moved before gst_init(), maybe? Seems to work (tm).
> 
> I wouldn't do it.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #8)
> (In reply to comment #7)
> > Which brings another question to me: if we change the default allocator, we
> > would be affecting other GStreamer multimedia playback outside WebKit. I'm
> > beginning to wonder about the consequences of this.
> > 
> 
> Yeah, this could be a bit of a blocker.
> 
> Also, Phil commented that our allocator should use the webkit_ prefix.

Why? it's a private object not exposed anywhere. I'm fine with using WebKit prefix in any case, but this is not like the web source element that other application might use. This is a private object only instantiated by webkit and set as default allocator in the web process when initializing gst.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #9)
> Turns out it's quite hard to get a GstAllocator implementation right.
> Instead, let's use FastMalloc in specific places as required, e.g. what bug
> #167928 does for MediaSourceClientGStreamerMSE.

I think we can give this another try. I've fixed some things in the patch and cleaned up a bit, I'll upload a new version now.

[Carlos Garcia Campos]

Created attachment 330544 [details]
Patch

[EWS Watchlist]

Attachment 330544 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:138:  gst_allocator_fast_malloc_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.cpp:145:  gst_allocator_fast_malloc_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/GstAllocatorFastMalloc.h:24:  gst_allocator_fast_malloc_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r226504: <https://trac.webkit.org/changeset/226504>

[Radar WebKit Bug Importer]

<rdar://problem/36345536>