Opened 10 years ago

Closed 7 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 tuan kuranes)

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)

Territory_Memfrag.patch (22.8 KB ) - added by tuan kuranes 10 years ago.
cleanedupTerr.patch (11.4 KB ) - added by wraitii 9 years ago.

Download all attachments as: .zip

Change History (20)

by tuan kuranes, 10 years ago

Attachment: Territory_Memfrag.patch added


comment:1 by tuan kuranes, 10 years ago

Description: modified (diff)
Priority: Should HaveNice to Have
Summary: Territory Memory fragmentation[PATCH] Territory Memory fragmentation

comment:2 by Kieran P, 10 years ago

Milestone: BacklogAlpha 15

comment:3 by Kieran P, 10 years ago

Milestone: Alpha 15Alpha 14

comment:4 by Kieran P, 10 years ago

Keywords: patch review added; memory fragmentation territory removed

comment:5 by scroogie, 10 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 historic_bruno, 9 years ago

Owner: set to tuan kuranes

comment:7 by Kieran P, 9 years ago

Milestone: Alpha 14Alpha 15

comment:8 by sanderd17, 9 years ago

Keywords: reviewed added; review removed
Milestone: Alpha 15Backlog

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 sanderd17, 9 years ago

Keywords: reviewed removed

by wraitii, 9 years ago

Attachment: cleanedupTerr.patch added

comment:10 by wraitii, 9 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 a few variables that otherwise are temporaries in CalculateTerritories() class members, and CalculateTerritories() is probably called often enough that this makes sense, even though it's not a huge improvement in term of memory fragmentation. It's not a code obfuscation either so I'm tempted to like it. 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.

Last edited 9 years ago by wraitii (previous) (diff)

comment:11 by wraitii, 9 years ago

Milestone: BacklogAlpha 16

comment:12 by leper, 9 years ago

Milestone: Alpha 16Alpha 17

comment:13 by historic_bruno, 9 years ago

Owner: tuan kuranes removed

comment:14 by Itms, 8 years ago

Milestone: Alpha 17Alpha 18

comment:15 by Itms, 8 years ago

Keywords: pathfinding added; review removed
Milestone: Alpha 18Alpha 19

Territory-related code is modified in the pathfinding branch, so putting this one with the others.

comment:16 by sanderd17, 8 years ago

Keywords: patch pathfinding removed
Resolution: fixed
Status: newclosed

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 fabio, 7 years ago

Resolution: fixed
Status: closedreopened

comment:18 by fabio, 7 years ago

Milestone: Alpha 19
Resolution: worksforme
Status: reopenedclosed
Note: See TracTickets for help on using tickets.