View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0000615||LDMud 3.3||Implementation||public||2009-03-13 17:45||2009-05-10 09:39|
|Target Version||3.3.719||Fixed in Version||3.3.719|
|Summary||0000615: RfC: Replace SINT in slaballoc.c/smalloc.c by sizeof(word_t)|
|Description||During tracing 0000611 I noticed that in slaballoc.c and smalloc.c the define SINT (SIZEOF_CHAR_P) is heavily used as the size of a word. I don't know the origin of SINT, but I think the semantic of it is not very apparant and there isn't even a comment.|
a) One suggestion is to replace it by a sizeof(word_t), because it is used as such. sizeof(word_t) is the same as SIZEOF_CHAR_P , because word_t is typedef'ed to p_uint and p_(u)int are integers with the same size as a pointer.
b) Another possibility is, to give SINT a new name which directly reflects meaning as sizeof(word_t). But I don't think, SIZOF_WORD_T is any better than sizeof(word_t).
c) And the third one: replace calculations with char* (e.g. block + 8*sizeof(word_t)) by the corresponding ones with word_t*, e.g. (word_t)block + 8, which is the most effort, but IMHO the cleanest approach. I would see it more as independent measure to a) or b).
: I am not sure, but I think the standard allows pointers to have different sizes, e.g. sizeof(void*)!=sizeof(int*). On systems like that we may be in rather bad shape...
Do you have any opinions about this?
|Tags||No tags attached.|
the comment about CHUNK_SIZE says: "make sure that the resulting chunk sizes are SINT aligned."
We may discuss replacing
#define MEM_ALIGN (SIZEOF_CHAR_P)
#define MEM_ALIGN (sizeof(word_t))
I don't like the idea of replacing a dedicated #define, however ill-named, by an anonymous sizeof().
How about renaming SINT to GRAN for "granularity"? We can then define MEM_ALIGN as GRAN.
I have no opinion on whether sizeof(word_t) is better than SIZEOF_CHAR_P.
slaballoc.c uses the same definitions, so whatever changes we decide on should be made in both smalloc.c and slaballoc.c.
Usually I would agree. But in this case I am not sure. Both allocators base on word_t:
* Allocations are measured in 'word_t's which are the same size as void*.
Which means, that you anyway can't change SINT/GRAN/whatever from being sizeof(word_t), because first you would have to decouple the rest of the file from word_t - which is a lot. And I would not see any reason to do so at the moment, because word_t is already an abstraction which can be changed.
We might decide not to base the allocations on void*/p_uint and increase/decrease granularity. But in any case we would IMHO just change word_t. (Ok, maybe something breaks in that case, because some code may rely on word_t being the size of void*/p_uint.)
Ok... The state is now, that we can't change any granularity and word_t independently.
Maybe de-coupling the two things is a nice idea, but I won't do now, because it is a lot of work and IMHO not that important (any other volounteers?).
I can use a better-named enum/define, but until someone checks the complete allocator, that will be IMHO very incomplete (and at some points may be even mis-used). Therefore I am not sure, if this does not lead to confusion (the false impression that we could change a granularity independently of word_t and that all uses of GRAN are correct). A comment may alleviate that somewhat. But for a long time (if not forever), any GRAN has to be sizeof(word_t).
Ok, I want this from my to-do list.
I replaced SINT by GRANULARITY and defined MEM_ALIGN to be GRANULARITY in r2575.
(BTW: I had to replace an #if by if, but as the expression is constant, the compiler will remove the if anyway.)
|2009-03-13 17:45||zesstra||New Issue|
|2009-03-13 17:45||zesstra||Status||new => assigned|
|2009-03-13 17:45||zesstra||Assigned To||=> zesstra|
|2009-03-13 17:52||zesstra||Note Added: 0000996|
|2009-03-13 19:14||fufu||Note Added: 0000998|
|2009-03-13 20:02||zesstra||Note Added: 0001000|
|2009-04-16 17:07||zesstra||Note Added: 0001050|
|2009-05-10 09:39||zesstra||Note Added: 0001089|
|2009-05-10 09:39||zesstra||Status||assigned => resolved|
|2009-05-10 09:39||zesstra||Fixed in Version||=> 3.3.719|
|2009-05-10 09:39||zesstra||Resolution||open => fixed|