WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
WIP
(9.58 KB, patch)
2016-12-13 04:46 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(10.67 KB, patch)
2018-01-05 03:17 PST
,
Carlos Garcia Campos
pnormand
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2016-12-13 01:35:55 PST
Created
attachment 296994
[details]
WIP
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
Created
attachment 297004
[details]
WIP
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
Created
attachment 330544
[details]
Patch
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
Committed
r226504
: <
https://trac.webkit.org/changeset/226504
>
Radar WebKit Bug Importer
Comment 17
2018-01-08 01:36:53 PST
<
rdar://problem/36345536
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug