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.
Created attachment 296994 [details] WIP
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
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.
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).
Created attachment 297004 [details] WIP
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.
(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.
(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.
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.
(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().
(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.
(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.
(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.
Created attachment 330544 [details] Patch
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.
Committed r226504: <https://trac.webkit.org/changeset/226504>
<rdar://problem/36345536>