09 Nov, 2008, Runter wrote in the 62nd comment:
Grimble said:
I'm not sure it was just about disk performance.
Memory pools are a common technique to address memory fragmentation. If you have software that periodically allocates/deallocates memory of variable size and let it run for a while, it will consume more total memory over time as the heap gets more and more fragmented.
Memory, storage, and CPU cycles are all much cheaper these days, so optimizations like this are far less critical but can still be beneficial.
Memory pools are a common technique to address memory fragmentation. If you have software that periodically allocates/deallocates memory of variable size and let it run for a while, it will consume more total memory over time as the heap gets more and more fragmented.
Memory, storage, and CPU cycles are all much cheaper these days, so optimizations like this are far less critical but can still be beneficial.
Where I'm going to have to dispute this is the flawed logic here. You're saving a small amount of ram by never freeing anything. So there's no real gain on actual memory because instead of freeing object or char_data pointers they're going into a linked list.
On a typical mud that can be as many as 15,000 - 50,000 objects and 5,000 to 15,000 mobiles. That's a lot of memory remaining resident when if you had freed it in the first place the fragmentation cost wouldn't have added up to the overhead of just maintaining these in memory. (Or the memory they use naturally.)
I agree that pool memory management has its place, but it's not for lowering memory costs. It's typically for more elegant (and often less efficient) code. Like an allocator object that can be associated with another object. Freeing the allocator object would dump all of the malloc() calls or new operations at once when the object it was attached to was no longer needed. (This would prevent any possible memory leaks since any element allocated was done through this memory allocator.) An example of this could be using an pool allocator object to allocate virtual zones or "instances" of zones and once the instance or virtual zone is ready to be freed, instead of having to go through and free everything contained within, you could simple dump the allocator object with a single free call to it. Without having to know where elements are allocated or valid, mobs, objects, rooms, scripts, anything.
(Keep in mind that rom's system doesn't support any of that. It doesn't even support freeing the elements if you wanted to.)
10 Nov, 2008, David Haley wrote in the 63rd comment:
Memory pools do indeed reduce fragmentation, which does indeed reduce overall memory usage. It's pretty easy to see this if you draw out a picture where you have fragmentation versus a picture where you don't. They also can, in some situations, improve performance, e.g. if allocation is expensive and the recycling of memory is not. Anyhow, if people are curious about this, there's plenty of literature around that examines the pros and cons; I'm not sure if it's really worth spending much time on here since it's not something that would really help us for various reasons.
10 Nov, 2008, Runter wrote in the 64th comment:
If they are reducing fragmentation at the cost of keeping many things you would have uniformly freed in memory then it isn't reducing the cost. The cost is all of the memory you didn't free, which can often cost more than what the fragmentation would have been if you don't plan on keeping your mud up for a month at a time.
So let's not put out misinformation.
So let's not put out misinformation.
10 Nov, 2008, David Haley wrote in the 65th comment:
It would help if you didn't interpret my statement as if it was made from the stupidest perspective possible. :smile: I didn't say that nothing should ever be freed… You were making a general statement about memory pools, saying that they do not help with fragmentation, and that just isn't true. Obviously it is possible to misuse them, but it's also possible to use them intelligently…
10 Nov, 2008, Runter wrote in the 66th comment:
DavidHaley said:
It would help if you didn't interpret my statement as if it was made from the stupidest perspective possible. :smile: I didn't say that nothing should ever be freed… You were making a general statement about memory pools, saying that they do not help with fragmentation, and that just isn't true. Obviously it is possible to misuse them, but it's also possible to use them intelligently…
Well, I just mostly was commenting on the way rom does their memory pool. One of the previous posters claimed that rom perhaps did it to save memory. So I just wanted to clear that up.
Memory pools in general are good devices, though. *uses boost::pool quite often*
10 Nov, 2008, David Haley wrote in the 67th comment:
Ah – I didn't realize we were only talking about what ROM does.
10 Nov, 2008, Sharmair wrote in the 68th comment:
Quote
rom doesn't have a char_died function. Also, calling ch->fighting is going to crash you if the pointer ch is no longer valid. E.g. it was freed or deleted.
The code I work with does char_died() on both the actor (ch) and victim after doing something that might
kill the char in a lot of places in violence_update() (which is a whole lot longer and does a lot more then
this one) and if ROM has some normal way to detect the same thing, it should be used here. But I am going
by the violence_update() code provided, and as it uses ch after multi_hit() (before the assist code), I
inferred that ch can not die in multi_hit(). That might be an issue, but for now I am thinking of the
current problem and don't want to confuse the poster any more then he already is.
Also, as a side note to this, the way that vch loop is used, it really does not need to hold the next, the only
place vch could die is in the multi_hit() and the loop exits if it is called. Also after some more thought, assuming you want to keep the code in violence_update() (you will have the same issues in multi_hit() BTW, and it could be argued as to the best place - violence_update() does the check only once per round
in auto combat, in multi_hit() it would also check on any direct calls to multi_hit(), including a recursive check on itself) I think doing a continue instead of adding the weapon code to a block might be better:
if (IS_AWAKE (ch) && ch->in_room == victim->in_room){
multi_hit (ch, victim, TYPE_UNDEFINED);
if ((victim = ch->fighting) == NULL)
continue;
/*
* Swords will very rarely get in an extra round.
*/
if (get_weapon_sn (ch) == gsn_sword && ch->wait <= 0)might fix your crash issue, then you can work on all the other issues
10 Nov, 2008, David Haley wrote in the 69th comment:
I thought that part of the problem here is that characters can die during multi_hit. If not there, then where else?
10 Nov, 2008, Runter wrote in the 70th comment:
I don't see where the loop is exiting if the pointer is no longer valid after multi_hit(). I see the pointer getting dereferenced at a spot where it may no longer be valid. (maybe to check if it is still valid?)
11 Nov, 2008, Sharmair wrote in the 71st comment:
Quote
I thought that part of the problem here is that characters can die during multi_hit. If not there, then where else?
Characters can die, that character being the VICTIM, the stock code provided assumes that the attacker can
not die there though. For now (as the problem he is having does not seem to be from that code) I will take
that as a given. Maybe there is the confusion as the debug output had the problem the room ch was in, but
in the problem call to can_see(), ch IS victim from violence_update() (it is checking if the victim can see
the attacker). And, again, if the code does have a way (maybe this valid data member) to detect a
character dieing, it should be used (victim->valid, ch->valid etc) after doing something that might kill
something. It might even be a good idea to use it on ch in violence_update() as someone might add
something to multi_hit() that might do damage to the attacker later. But again, I am trying to get the
current problem fixed before going on to design issues that could be a problem later.
11 Nov, 2008, Sharmair wrote in the 72nd comment:
Quote
I don't see where the loop is exiting if the pointer is no longer valid after multi_hit().
From his code post of his violence_update(), last part of his weapons code:
for (vch = ch->in_room->people; vch != NULL; vch = vch->next_in_room) {
if (vch->fighting == ch || is_same_group (vch->fighting, ch)) {
if (number_percent () < chance) {
was_fighting = ch->fighting;
ch->fighting = vch;
multi_hit (ch, vch, TYPE_UNDEFINED);
check_improve (ch, gsn_sword, TRUE, 2);
ch->fighting = was_fighting;
WAIT_STATE (ch, PULSE_VIOLENCE);
break; //exit after finding a victim
}
}
}After multi_hit() is called with vch being the victim, vch is not used again as the loop is exited with the
break.
11 Nov, 2008, David Haley wrote in the 73rd comment:
But that's the point, right? Looking at the code you pasted just now, it breaks after finding a victim, right after a call to multi_hit. So the victim might have just died…
12 Nov, 2008, Igabod wrote in the 74th comment:
Sandi said:
This:
should be this:
Drop by Tir na nOg and see what I've done in 5 years. I don't know any C, either. ;)
Before we try to explain why the above is needed, do you understand how a for() loop works?
for (vch = ch->in_room->people; vch != NULL; vch = vch->next_in_room)
should be this:
for (vch = ch->in_room->people; vch != NULL; vch = vch_next)
vch_next = vch->next_in_room;
Drop by Tir na nOg and see what I've done in 5 years. I don't know any C, either. ;)
Before we try to explain why the above is needed, do you understand how a for() loop works?
did that and declared vch_next and it crashes still. gdb shows this:
Program received signal SIGSEGV, Segmentation fault.
0x004376b0 in violence_update () at fight.c:113
113 if (vch->fighting == ch
(gdb) tb
Breakpoint 1 at 0x4376b0: file fight.c, line 113.
(gdb) bt
#0 0x004376b0 in violence_update () at fight.c:113
#1 0x00479f2b in update_handler () at update.c:1199
#2 0x0042c442 in game_loop_unix (control=5) at comm.c:844
#3 0x0042bed8 in main (argc=2, argv=0x10e19b0) at comm.c:448
(gdb) list
108 chance = chance * 3 / 5;
109
110 for (vch = ch->in_room->people; vch != N
ULL; vch = vch_next)
111 vch_next = vch->next_in_room;
112 {
113 if (vch->fighting == ch
114 || is_same_group (vch->f
ighting, ch))
115 {
116 if (number_percent () <
chance)
117 {
(gdb)
funny thing is it got past the can_see and crashed in the vch_next portion right after it.
12 Nov, 2008, David Haley wrote in the 75th comment:
What's the value of vch – is it a valid pointer? Does it point to bogus memory? etc.
12 Nov, 2008, Igabod wrote in the 76th comment:
i'm not sure how to find that out but i'll guess you mean how is it declared in the function? i declared it as CHAR_DATA *vch_next;
12 Nov, 2008, Sharmair wrote in the 77th comment:
DavidHaley said:
But that's the point, right? Looking at the code you pasted just now, it breaks after finding a victim, right after a call to multi_hit. So the victim might have just died…
Yes, in that code, the victim of multi_hit() (vch in this case) might have died, but as the loop is exited
and in effect, vch (or at least the loop iteration) goes out of scope, it does not matter. The point is that
as the loop is not continued after the loop pointer might have gone invalid, a vch_next holder is not needed.
It might be argued that a new coder might not be able to determine when a holder is needed and should
use one all the time. But this was brought up as a side issue (that had nothing to do with his original
problem) and was just one more thing that has muddled the whole issue.
12 Nov, 2008, Sharmair wrote in the 78th comment:
Igabod said:
i'm not sure how to find that out but i'll guess you mean how is it declared in the function? i declared it as CHAR_DATA *vch_next;
If you are in gdb, you can use 'print vch' to see what the pointer itself is, and 'print *vch' to see the thing
it is pointed to, if it is NULL, vch will show as 0x0 and if it is out of addressable range, when you do the
later is should give some out of bounds message. if you get the later, you can look at all the data in the
data (the pointer to CHAR_DATA in this case) and see if the data looks good. You can also see the data
like name (if still valid) and know WHAT character it is and the like.
But, in this case, the error is quite obvious. The code you changed was in error as written, you are
looping through all the characters in the room, THEN using the NULL end of loop vch. I suppose now
that you have added the vch_next holder, you might as well keep it, but move the { you have after
the vch_next = vch->next_in_room; to the end of the for(vch = ch->in_room->people; vch != NULL; vch = vch_next) line.
12 Nov, 2008, Igabod wrote in the 79th comment:
just the { ??? wont that cause an error?
12 Nov, 2008, Sharmair wrote in the 80th comment:
Igabod said:
just the { ??? wont that cause an error?
No, not if you just move it up a line like I said. A lot of C/C++ statements (like for, if, while etc) have a
syntax that takes a statement after them (the closing ')'). If you have more then one, you use a
compound statement (ie. you enclose them in {}). In your case here, the for loop only has the setting
of the next in it and nothing else:
for (vch = ch->in_room->people; vch != NULL; vch = vch_next)
vch_next = vch->next_in_room;
Is your whole loop.
After the for statement, vch is NULL, the exit condition. Then you define a scope
(the use of the {} after this) where you TRY to dereference vch and being it is NULL,
you get a crash. If it did not crash, that break would have broke you out of the main
violence_update() loop (skipping all the rest of the characters in the game after the current
ch).
In short, if you are going to use that vch_next holder, it should be in the loop's compound
statement, like:
for(vch = ch->in_room->people; vch != NULL; vch = vch_next){
vch_next = vch->next_in_room;
//…rest if loop here (those 2 ifs)
}Just to make it clear, you just have to MOVE the one { not add or delete any.
Random Picks
I'll take all the credit for that.