GNOME Bugzilla – Bug 739440
GstGLMemory lazy malloc system memory
Last modified: 2016-09-22 08:08:19 UTC
Created attachment 289714 [details] [review] misc patch to lazy malloc glmemory's system memory gst_gl_memory_alloc always allocate system memory by g_malloc, but this memory is only used when map buffer for system memory read/write, should this memory be lazy allocated only if in this case ? attachment is a misc patch to do this, but out of memory can only be detected when map buffer for system read/write, not the allocate time.
Review of attachment 289714 [details] [review]: Just on thread safety comment :) Looks good otheriwise. ::: gst-libs/gst/gl/gstglmemory.c @@ +661,3 @@ + + if (!gl_mem->data) { + gl_mem->data = g_malloc (gl_mem->mem.maxsize); Nothing stops the map function being called concurrently so this needs to be protected by either a GOnce or a mutex to avoid accidentally double allocating.
Created attachment 290131 [details] [review] misc patch to lazy malloc glmemory's system memory (In reply to comment #1) > Review of attachment 289714 [details] [review]: > > Just on thread safety comment :) > > Nothing stops the map function being called concurrently so this needs to be > protected by either a GOnce or a mutex to avoid accidentally double allocating. use g_once to avoid double allocating, also lazy allocate memory in copy codepath (NEEDS_DOWNLOAD=true but not mapped for system R/W yet). not familiar with g_once, please point out my problem if any :)
Created attachment 291618 [details] [review] misc patch to lazy malloc glmemory's system memory I misunderstood g_once totally so previous patch is broken (map for system memory didn't work at all). this one use mutex instead and worked as I test. sorry for previous incorrect one.
The patch no longer applies due to changes from f2dc9b2b8f9b8b180f41e0ae88d52229653c21b4, b8f168cd65fbdc85199bfb4eca2afd1445215880 and others. However, _gl_mem_alloc_data is still being performed in: * _gl_mem_copy * gst_gl_memory_wrapped_texture * gst_gl_memory_alloc and not lazily postponed to mem_map, so refreshing that patch still sounds like a good idea.
One note, this is virtual memory, so the kernel already is doing lazy allocation, and we can't know if we are out of memory. I don't think this change will make any difference.
That's a good point. I suppose it won't make difference in physical memory usage. If anything, we should care about the malloc time. In some pipelines, upload and download never happens so the malloc serves no purpose. Can't say I've done any profiling to prove the need for this optimization, though :)
My comment is mostly for "but out of memory can only be detected when map" since we can't be sure if the memory is available or not. It can't hurt not doing a malloc needlessly. Though as said, I doubt the benchmarks will show anything significant.
this patch is based on googleperftool malloc profile result ( not cpu performance benchmark), it can only reduce the malloc/free times and gives less total malloc size. since I haven't read g_malloc/malloc/kernel detail ,I'm not sure the underlying difference (maybe it can reduce some book-keeping in multi-level allocator? ).
This seems to have been solved in the transition to glbasememory. gst_gl_base_memory_alloc_data() (which controls the sysmem allocation) is only called when necessary from the memcpy implementation or from the cpu accesses of the dependant classes. commit 27e724df8f0b7bb0d98a6c1589777633b3dadcba Author: Matthew Waters <matthew@centricular.com> Date: Mon Dec 14 12:59:02 2015 +1100 gl: add a base memory object It handles the following - GstAllocationParams -> gst_memory_init transformation - Makes sure that map/unmap/create/destroy happen on the GL thread with a GL context current. - Holds a possible sysmem accessible data pointer with alignment. - Holds the need upload/download transfer state