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 |
| Reporter | zesstra | Assigned To | zesstra | ||
| Priority | normal | Severity | minor | Reproducibility | N/A |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.3.718 | ||||
| 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 [0], 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). [0]: 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. | ||||
|
|
BTW: 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) by #define MEM_ALIGN (sizeof(word_t)) as well. |
|
|
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.) |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 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 |
