[Mesa-dev] [PATCH 01/10] glsl: memory_writer helper class for data serialization
Paul Berry
stereotype441 at gmail.com
Tue Feb 4 20:13:02 PST 2014
On 4 February 2014 19:52, Paul Berry <stereotype441 at gmail.com> wrote:
> Whoops, I discovered another issue:
>
>
> On 29 January 2014 01:24, Tapani Pälli <tapani.palli at intel.com> wrote:
>
>> Class will be used by the shader binary cache implementation.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>> src/glsl/memory_writer.h | 188
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 188 insertions(+)
>> create mode 100644 src/glsl/memory_writer.h
>>
>> diff --git a/src/glsl/memory_writer.h b/src/glsl/memory_writer.h
>> new file mode 100644
>> index 0000000..979169f
>> --- /dev/null
>> +++ b/src/glsl/memory_writer.h
>> @@ -0,0 +1,188 @@
>> +/* -*- c++ -*- */
>> +/*
>> + * Copyright © 2013 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining
>> a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> next
>> + * paragraph) shall be included in all copies or substantial portions of
>> the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#pragma once
>> +#ifndef MEMORY_WRITER_H
>> +#define MEMORY_WRITER_H
>> +
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +#include <string.h>
>> +
>> +#include "main/hash_table.h"
>> +
>> +#ifdef __cplusplus
>> +/**
>> + * Helper class for writing data to memory
>> + *
>> + * This class maintains a dynamically-sized memory buffer and allows
>> + * for data to be efficiently appended to it with automatic resizing.
>> + */
>> +class memory_writer
>> +{
>> +public:
>> + memory_writer() :
>> + memory(NULL),
>> + curr_size(0),
>> + pos(0)
>> + {
>> + data_hash = _mesa_hash_table_create(0, int_equal);
>> + hash_value = _mesa_hash_data(this, sizeof(memory_writer));
>> + }
>> +
>> + ~memory_writer()
>> + {
>> + free(memory);
>> + _mesa_hash_table_destroy(data_hash, NULL);
>> + }
>> +
>> + /* user wants to claim the memory */
>> + char *release_memory(size_t *size)
>> + {
>> + /* final realloc to free allocated but unused memory */
>> + char *result = (char *) realloc(memory, pos);
>> + *size = pos;
>> + memory = NULL;
>> + curr_size = 0;
>> + pos = 0;
>> + return result;
>> + }
>> +
>> +/**
>> + * write functions per type
>> + */
>> +#define DECL_WRITER(type) void write_ ##type (const type data) {\
>> + write(&data, sizeof(type));\
>> +}
>> +
>> + DECL_WRITER(int32_t);
>> + DECL_WRITER(int64_t);
>> + DECL_WRITER(uint8_t);
>> + DECL_WRITER(uint32_t);
>> +
>> + void write_bool(bool data)
>> + {
>> + uint8_t val = data;
>> + write_uint8_t(val);
>> + }
>> +
>> + /* write function that reallocates more memory if required */
>> + void write(const void *data, int size)
>> + {
>> + if (!memory || pos > (curr_size - size))
>> + if (!grow(size))
>> + return;
>> +
>> + memcpy(memory + pos, data, size);
>> +
>> + pos += size;
>> + }
>> +
>> + void overwrite(const void *data, int size, int offset)
>> + {
>> + if (offset < 0 || offset + size > pos)
>> + return;
>> + memcpy(memory + offset, data, size);
>> + }
>> +
>> + /* length is written to make reading safe */
>> + void write_string(const char *str)
>> + {
>> + uint32_t len = str ? strlen(str) : 0;
>> + write_uint32_t(len);
>> +
>> + if (str)
>> + write(str, len + 1);
>> + }
>> +
>> + unsigned position()
>> + {
>> + return pos;
>> + }
>> +
>> + /**
>> + * check if some data was written
>> + */
>> + bool data_was_written(void *data, uint32_t id)
>> + {
>> + hash_entry *entry =
>> + _mesa_hash_table_search(data_hash, hash_value,
>> + (void*) (intptr_t) id);
>>
>
> This isn't an efficient way to use a hashtable. You're always passing the
> same hash_value (the one computed in the constructor), which means that
> every single hash entry will be stored in the same chain, and lookups will
> be really slow.
>
> Since you're only using this hashtable to detect duplicate glsl_types,
> there's a much easier approach: take advantage of the fact that glsl_type
> is a flyweight class (in other words, code elsewhere in Mesa ensures that
> no duplicate glsl_type objects exist anywhere in memory, so any two
> glsl_type pointers can be compared simply using pointer equality). Because
> of that, you should be able to just use a pointer-based hashtable, like
> ir_variable_refcount_visitor does. In short, that means:
>
> - Get rid of the hash_value class member.
>
> - In the constructor, do this: _mesa_hash_table_create(NULL,
> _mesa_key_pointer_equal)
>
> - The data_was_written() function can just look like this:
>
> bool data_was_written(void *data)
> {
> return _mesa_hash_table_search(data_hash, _mesa_hash_pointer(data),
> data) != NULL;
> }
>
> - The mark_data_written() function can just look like this:
>
> void mark_data_written(void *data)
> {
> _mesa_hash_table_insert(data_hash, _mesa_hash_pointer(data),
> data, NULL);
> }
>
> Note that I'm passing NULL for the last argument of
> _mesa_hash_table_insert because we don't actually need the hashtable to act
> as a key/value store; we are just using it to check whether we've seen a
> pointer before; so when we're marking that we've seen a pointer we don't
> need to store any data with it.
>
Oops, sorry, I was wrong about this. We do actually need the hashtable to
act as a key/value store, because we need it to store the mapping from
"data" to the unique ID. However, the problem with what you've written is
that there's nothing to guarantee that the ID is unique. (In patch 2, when
you call these functions, you form the ID by calling _mesa_hash_data() on
the contents of the glsl_type, but that's not safe because two distinct
glsl_types might hash to the same value).
How about if we replace data_was_written() and mark_data_written() with
something like this:
/**
* Convert the given pointer into a small integer unique ID. In other
* words, if make_unique_id() has previously been called with this
pointer,
* return the same ID that was returned last time. If this is the first
* call to make_unique_id() with this pointer, return a fresh ID.
*
* Return value is true if the pointer has been seen before, false
* otherwise.
*/
bool make_unique_id(void *ptr, uint32_t *id_out)
{
void *entry =
_mesa_hash_table_search(data_hash, _mesa_hash_pointer(ptr), ptr);
if (entry != NULL) {
*id_out = (uint32_t) (intptr_t) entry;
return true;
} else {
/* Note: hashtable uses 0 to represent "entry not found" so our
* unique ID's need to start at 1. Hence, preincrement
* unique_id_counter.
*/
*id_out = ++this->unique_id_counter;
_mesa_hash_table_insert(data_hash, _mesa_hash_pointer(ptr), ptr,
(void *) (intptr_t) *id_out);
return false;
}
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140204/7a6b1c6a/attachment.html>
More information about the mesa-dev
mailing list