RESOLVED FIXED Bug 165793
[GStreamer] use FastMalloc-based GstAllocator
https://bugs.webkit.org/show_bug.cgi?id=165793
Summary [GStreamer] use FastMalloc-based GstAllocator
Zan Dobersek
Reported 2016-12-13 01:35:01 PST
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.
Attachments
WIP (9.48 KB, patch)
2016-12-13 01:35 PST, Zan Dobersek
no flags
WIP (9.58 KB, patch)
2016-12-13 04:46 PST, Zan Dobersek
no flags
Patch (10.67 KB, patch)
2018-01-05 03:17 PST, Carlos Garcia Campos
pnormand: review+
Zan Dobersek
Comment 1 2016-12-13 01:35:55 PST
Xabier Rodríguez Calvar
Comment 2 2016-12-13 03:38:29 PST
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 3 2016-12-13 04:25:36 PST
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 4 2016-12-13 04:42:29 PST
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
Comment 5 2016-12-13 04:46:01 PST
Enrique Ocaña
Comment 6 2016-12-13 05:05:16 PST
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
Comment 7 2016-12-13 08:15:57 PST
(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
Comment 8 2016-12-27 10:20:23 PST
(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
Comment 9 2017-02-07 09:55:28 PST
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
Comment 10 2018-01-05 03:06:02 PST
(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
Comment 11 2018-01-05 03:08:58 PST
(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
Comment 12 2018-01-05 03:10:31 PST
(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
Comment 13 2018-01-05 03:11:41 PST
(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
Comment 14 2018-01-05 03:17:33 PST
EWS Watchlist
Comment 15 2018-01-05 03:19:56 PST
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
Comment 16 2018-01-08 01:34:45 PST
Radar WebKit Bug Importer
Comment 17 2018-01-08 01:36:53 PST
Note You need to log in before you can comment on or make changes to this bug.