Opened 11 years ago
Closed 8 years ago
#2022 closed defect (worksforme)
[PATCH] Territory Memory fragmentation
Reported by: | tuan kuranes | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description (last modified by )
Each call to territory related function involves lots of temporary, mostly std::vector, sometimes unecessary std::vectors.
This very basic patch just aims to keep them between calls. (which are very numerous, and can be triggered by javascript calls.)
A better patch would follow a "memory pool" way, as pointed in forums, so this patch may not be very interesting in the end and regarding RedFox amazing work on that topic, the patch may not be useful at all there.
Attachments (2)
Change History (20)
by , 11 years ago
Attachment: | Territory_Memfrag.patch added |
---|
comment:1 by , 11 years ago
Description: | modified (diff) |
---|---|
Priority: | Should Have → Nice to Have |
Summary: | Territory Memory fragmentation → [PATCH] Territory Memory fragmentation |
comment:2 by , 11 years ago
Milestone: | Backlog → Alpha 15 |
---|
comment:3 by , 11 years ago
Milestone: | Alpha 15 → Alpha 14 |
---|
comment:4 by , 11 years ago
Keywords: | patch review added; memory fragmentation territory removed |
---|
comment:5 by , 11 years ago
It would be easier to read you patches if you separated unrelated changes, like in this patch the const changes etc. from the permanent variables instead of temporaries, as it is easier to get an overview then. The changes look very sensible, though. They basically just replace temporary variables with class members afaics.
comment:6 by , 11 years ago
Owner: | set to |
---|
comment:7 by , 11 years ago
Milestone: | Alpha 14 → Alpha 15 |
---|
comment:8 by , 10 years ago
Keywords: | reviewed added; review removed |
---|---|
Milestone: | Alpha 15 → Backlog |
Sorry, but with all those unnecessary changes, your patch is very hard to review. If you can separate the core fix from all other code changes, we could reconsider this patch. But for now, I'm gonna take it out of the review queue.
comment:9 by , 10 years ago
Keywords: | reviewed removed |
---|
by , 10 years ago
Attachment: | cleanedupTerr.patch added |
---|
comment:10 by , 10 years ago
Keywords: | review added |
---|
Cleaned up Kuranes' patch, removing unnecessary consts, and a few changes which I deemed useless too (this is to my liking, but since nobody was reviewing the above patch). The idea behind the patch is simply to make class members a few variables that otherwise are temporaries in CalculateTerritories(), which is probably called often enough that this makes sense, even though it's not a huge improvement. I've added a few comments starting with "wraitii" to explain a few things. I've inlined the grid functions, and left those he added though we likely don't need them. Kuranes had a liking to save the end of loops in a local variable just in front of the loop. It's fairly ugly, might be or might not be faster (should check assembly code). I've kept most of them.
This change was still done fairly quickly so I haven't really checked that pointer behavior is sound and there may be segfaults. But it should be reviewable.
edit: I've noticed I forgot to remove the last remnants of the grid32Pool variable. It's now useless as it hardly had a purpose.
comment:11 by , 10 years ago
Milestone: | Backlog → Alpha 16 |
---|
comment:12 by , 10 years ago
Milestone: | Alpha 16 → Alpha 17 |
---|
comment:13 by , 10 years ago
Owner: | removed |
---|
comment:14 by , 10 years ago
Milestone: | Alpha 17 → Alpha 18 |
---|
comment:15 by , 9 years ago
Keywords: | pathfinding added; review removed |
---|---|
Milestone: | Alpha 18 → Alpha 19 |
Territory-related code is modified in the pathfinding branch, so putting this one with the others.
comment:16 by , 9 years ago
Keywords: | patch pathfinding removed |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Memory fragmentation has improved a lot in the recent territory rewrite, next to general algorithm improvements.
Which makes this patch unneeded and hard to apply.
comment:17 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:18 by , 8 years ago
Milestone: | Alpha 19 |
---|---|
Resolution: | → worksforme |
Status: | reopened → closed |
patch