On 2014-11-05 00:26, Peter Meerwald wrote:
> From: Peter Meerwald <p.meerwald at bct-electronic.com>
>
> item_info has per-type fields which should be within a union to
> save space
I think this is mostly a bikeshed patch, because the memory saved can't
be significant, can it?
If the memory saved is insignificant I'd say that just longer code
(added "per_type.memblock_info" here and there) is a slight drawback and
hence this patch could be skipped. But it's not a strong opinion.
>
> Signed-off-by: Peter Meerwald <pmeerw at pmeerw.net>
> ---
> src/pulsecore/pstream.c | 117 +++++++++++++++++++++++++++---------------------
> 1 file changed, 65 insertions(+), 52 deletions(-)
>
> diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
> index 96ee247..f8217b3 100644
> --- a/src/pulsecore/pstream.c
> +++ b/src/pulsecore/pstream.c
> @@ -92,21 +92,26 @@ struct item_info {
> PA_PSTREAM_ITEM_SHMREVOKE
> } type;
>
> - /* packet info */
> - pa_packet *packet;
> + union {
> + /* packet info */
> + pa_packet *packet;
> +
> + /* release/revoke info */
> + uint32_t block_id;
> +
> + /* memblock info */
> + struct {
> + pa_memchunk chunk;
> + uint32_t channel;
> + int64_t offset;
> + pa_seek_mode_t seek_mode;
> + } memblock_info;
> + } per_type;
> +
> #ifdef HAVE_CREDS
> bool with_ancil_data;
> pa_cmsg_ancil_data ancil_data;
> #endif
> -
> - /* memblock info */
> - pa_memchunk chunk;
> - uint32_t channel;
> - int64_t offset;
> - pa_seek_mode_t seek_mode;
> -
> - /* release/revoke info */
> - uint32_t block_id;
> };
>
> struct pstream_read {
> @@ -285,11 +290,11 @@ static void item_free(void *item) {
> pa_assert(i);
>
> if (i->type == PA_PSTREAM_ITEM_MEMBLOCK) {
> - pa_assert(i->chunk.memblock);
> - pa_memblock_unref(i->chunk.memblock);
> + pa_assert(i->per_type.memblock_info.chunk.memblock);
> + pa_memblock_unref(i->per_type.memblock_info.chunk.memblock);
> } else if (i->type == PA_PSTREAM_ITEM_PACKET) {
> - pa_assert(i->packet);
> - pa_packet_unref(i->packet);
> + pa_assert(i->per_type.packet);
> + pa_packet_unref(i->per_type.packet);
> }
>
> if (pa_flist_push(PA_STATIC_FLIST_GET(items), i) < 0)
> @@ -324,25 +329,19 @@ static void pstream_free(pa_pstream *p) {
> pa_xfree(p);
> }
>
> -void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const pa_cmsg_ancil_data *ancil_data) {
> - struct item_info *i;
> -
> +static void pa_pstream_send_item(pa_pstream*p, struct item_info *item, const pa_cmsg_ancil_data *ancil_data) {
It looks like pa_pstream_send_item is only used for
pa_pstream_send_packet and not for pa_pstream_send_memblock (etc), how come?
> pa_assert(p);
> pa_assert(PA_REFCNT_VALUE(p) > 0);
> - pa_assert(packet);
> + pa_assert(item);
>
> - if (p->dead)
> + if (p->dead) {
> + item_free(item);
> return;
> -
> - if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> - i = pa_xnew(struct item_info, 1);
> -
> - i->type = PA_PSTREAM_ITEM_PACKET;
> - i->packet = pa_packet_ref(packet);
> + }
>
> #ifdef HAVE_CREDS
> - if ((i->with_ancil_data = !!ancil_data)) {
> - i->ancil_data = *ancil_data;
> + if ((item->with_ancil_data = !!ancil_data)) {
> + item->ancil_data = *ancil_data;
> if (ancil_data->creds_valid)
> pa_assert(ancil_data->nfd == 0);
> else
> @@ -350,11 +349,25 @@ void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const pa_cmsg_ancil
> }
> #endif
>
> - pa_queue_push(p->send_queue, i);
> + pa_queue_push(p->send_queue, item);
>
> p->mainloop->defer_enable(p->defer_event, 1);
> }
>
> +void pa_pstream_send_packet(pa_pstream*p, pa_packet *packet, const pa_cmsg_ancil_data *ancil_data) {
> + struct item_info *i;
> +
> + pa_assert(packet);
> +
> + if (!(i = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> + i = pa_xnew(struct item_info, 1);
> +
> + i->type = PA_PSTREAM_ITEM_PACKET;
> + i->per_type.packet = pa_packet_ref(packet);
> +
> + pa_pstream_send_item(p, i, ancil_data);
> +}
> +
> void pa_pstream_send_memblock(pa_pstream*p, uint32_t channel, int64_t offset, pa_seek_mode_t seek_mode, const pa_memchunk *chunk) {
> size_t length, idx;
> size_t bsm;
> @@ -381,13 +394,13 @@ void pa_pstream_send_memblock(pa_pstream*p, uint32_t channel, int64_t offset, pa
> i->type = PA_PSTREAM_ITEM_MEMBLOCK;
>
> n = PA_MIN(length, bsm);
> - i->chunk.index = chunk->index + idx;
> - i->chunk.length = n;
> - i->chunk.memblock = pa_memblock_ref(chunk->memblock);
> + i->per_type.memblock_info.chunk.index = chunk->index + idx;
> + i->per_type.memblock_info.chunk.length = n;
> + i->per_type.memblock_info.chunk.memblock = pa_memblock_ref(chunk->memblock);
>
> - i->channel = channel;
> - i->offset = offset;
> - i->seek_mode = seek_mode;
> + i->per_type.memblock_info.channel = channel;
> + i->per_type.memblock_info.offset = offset;
> + i->per_type.memblock_info.seek_mode = seek_mode;
> #ifdef HAVE_CREDS
> i->with_ancil_data = false;
> #endif
> @@ -414,7 +427,7 @@ void pa_pstream_send_release(pa_pstream *p, uint32_t block_id) {
> if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> item = pa_xnew(struct item_info, 1);
> item->type = PA_PSTREAM_ITEM_SHMRELEASE;
> - item->block_id = block_id;
> + item->per_type.block_id = block_id;
> #ifdef HAVE_CREDS
> item->with_ancil_data = false;
> #endif
> @@ -451,7 +464,7 @@ void pa_pstream_send_revoke(pa_pstream *p, uint32_t block_id) {
> if (!(item = pa_flist_pop(PA_STATIC_FLIST_GET(items))))
> item = pa_xnew(struct item_info, 1);
> item->type = PA_PSTREAM_ITEM_SHMREVOKE;
> - item->block_id = block_id;
> + item->per_type.block_id = block_id;
> #ifdef HAVE_CREDS
> item->with_ancil_data = false;
> #endif
> @@ -495,9 +508,9 @@ static void prepare_next_write_item(pa_pstream *p) {
> if (p->write.current->type == PA_PSTREAM_ITEM_PACKET) {
> size_t plen;
>
> - pa_assert(p->write.current->packet);
> + pa_assert(p->write.current->per_type.packet);
>
> - p->write.data = (void *) pa_packet_data(p->write.current->packet, &plen);
> + p->write.data = (void *) pa_packet_data(p->write.current->per_type.packet, &plen);
> p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] = htonl((uint32_t) plen);
>
> if (plen <= MINIBUF_SIZE - PA_PSTREAM_DESCRIPTOR_SIZE) {
> @@ -508,32 +521,32 @@ static void prepare_next_write_item(pa_pstream *p) {
> } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMRELEASE) {
>
> p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] = htonl(PA_FLAG_SHMRELEASE);
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl(p->write.current->block_id);
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl(p->write.current->per_type.block_id);
>
> } else if (p->write.current->type == PA_PSTREAM_ITEM_SHMREVOKE) {
>
> p->write.descriptor[PA_PSTREAM_DESCRIPTOR_FLAGS] = htonl(PA_FLAG_SHMREVOKE);
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl(p->write.current->block_id);
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl(p->write.current->per_type.block_id);
>
> } else {
> uint32_t flags;
> bool send_payload = true;
>
> pa_assert(p->write.current->type == PA_PSTREAM_ITEM_MEMBLOCK);
> - pa_assert(p->write.current->chunk.memblock);
> + pa_assert(p->write.current->per_type.memblock_info.chunk.memblock);
>
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] = htonl(p->write.current->channel);
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl((uint32_t) (((uint64_t) p->write.current->offset) >> 32));
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] = htonl((uint32_t) ((uint64_t) p->write.current->offset));
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_CHANNEL] = htonl(p->write.current->per_type.memblock_info.channel);
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_HI] = htonl((uint32_t) (((uint64_t) p->write.current->per_type.memblock_info.offset) >> 32));
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_OFFSET_LO] = htonl((uint32_t) ((uint64_t) p->write.current->per_type.memblock_info.offset));
>
> - flags = (uint32_t) (p->write.current->seek_mode & PA_FLAG_SEEKMASK);
> + flags = (uint32_t) (p->write.current->per_type.memblock_info.seek_mode & PA_FLAG_SEEKMASK);
>
> if (p->use_shm) {
> uint32_t block_id, shm_id;
> size_t offset, length;
> uint32_t *shm_info = (uint32_t *) &p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
> size_t shm_size = sizeof(uint32_t) * PA_PSTREAM_SHM_MAX;
> - pa_mempool *current_pool = pa_memblock_get_pool(p->write.current->chunk.memblock);
> + pa_mempool *current_pool = pa_memblock_get_pool(p->write.current->per_type.memblock_info.chunk.memblock);
> pa_memexport *current_export;
>
> if (p->mempool == current_pool)
> @@ -542,7 +555,7 @@ static void prepare_next_write_item(pa_pstream *p) {
> pa_assert_se(current_export = pa_memexport_new(current_pool, memexport_revoke_cb, p));
>
> if (pa_memexport_put(current_export,
> - p->write.current->chunk.memblock,
> + p->write.current->per_type.memblock_info.chunk.memblock,
> &block_id,
> &shm_id,
> &offset,
> @@ -555,8 +568,8 @@ static void prepare_next_write_item(pa_pstream *p) {
>
> shm_info[PA_PSTREAM_SHM_BLOCKID] = htonl(block_id);
> shm_info[PA_PSTREAM_SHM_SHMID] = htonl(shm_id);
> - shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset + p->write.current->chunk.index));
> - shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t) p->write.current->chunk.length);
> + shm_info[PA_PSTREAM_SHM_INDEX] = htonl((uint32_t) (offset + p->write.current->per_type.memblock_info.chunk.index));
> + shm_info[PA_PSTREAM_SHM_LENGTH] = htonl((uint32_t) p->write.current->per_type.memblock_info.chunk.length);
>
> p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] = htonl(shm_size);
> p->write.minibuf_validsize = PA_PSTREAM_DESCRIPTOR_SIZE + shm_size;
> @@ -569,8 +582,8 @@ static void prepare_next_write_item(pa_pstream *p) {
> }
>
> if (send_payload) {
> - p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] = htonl((uint32_t) p->write.current->chunk.length);
> - p->write.memchunk = p->write.current->chunk;
> + p->write.descriptor[PA_PSTREAM_DESCRIPTOR_LENGTH] = htonl((uint32_t) p->write.current->per_type.memblock_info.chunk.length);
> + p->write.memchunk = p->write.current->per_type.memblock_info.chunk;
> pa_memblock_ref(p->write.memchunk.memblock);
> p->write.data = NULL;
> }
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
