Page MenuHomeWildfire Games

Cleanup of CCmprangeManager
ClosedPublic

Authored by mimo on Jan 28 2017, 2:25 PM.

Details

Summary

Following discussions in https://code.wildfiregames.com/D60 the use of u8 to store each boolean has been cleanup up. Now only one u8 is used, thus reducing the size of the struct.

Test Plan

checked that everything works as before (including serialization).

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 455
Build 735: Vulcan BuildJenkins
Build 734: arc lint + arc unit

Event Timeline

mimo created this revision.Jan 28 2017, 2:25 PM
Owners added a subscriber: Restricted Owners Package.Jan 28 2017, 2:25 PM
mimo updated this revision to Diff 369.Jan 28 2017, 2:48 PM

improved version

Vulcan added a subscriber: Vulcan.Jan 28 2017, 3:10 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/256/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/257/ for more details.

I guess the HasProperty and SetProperty functions could be done using templates, as there is no case where we want to look up the property dynamically. We could also pack properties by having that as an extensible (template-packed) structure where lookups are just looking for the correct nesting, which takes N bits.

Alternatively we could use bitfields, which would make the code more readable without all those bit twiddling operations, and remove the need for the enum.

source/simulation2/components/CCmpRangeManager.cpp
196

We should ensure that this actually starts from 0, and most likely also ensure that there aren't any strange jumps (not that any sane compiler would do that).

219

I guess we can also limit that if we really want to.

220

2*9 = 18, so quite some space if we really want to pack things.

221

I guess there is some possible gain here too.

222

We only really need 9 of those 16 bits, so we do have at least 7 unused bits here. (8 players and gaia, everything else is already limited in many other places.)

223–224

Note that we do currently have 2 flags, so we could also merge that into the properties, but that would be a bit strange with the current code.

223–224

IIRC owner doesn't even use close to the full range of an i8 and could easily be done without complicating anything by using 5 bits (so -1 can be represented without any ugly hacks).

234

Even though this is fine given the operator precedence, but I guess someone else reading this might have to look up that, so a few () might help readability.

1702

Seems unrelated, but I guess that is ok.

mimo added a comment.Jan 29 2017, 11:56 AM
In D101#3545, @leper wrote:

I guess the HasProperty and SetProperty functions could be done using templates, as there is no case where we want to look up the property dynamically. We could also pack properties by having that as an extensible (template-packed) structure where lookups are just looking for the correct nesting, which takes N bits.

Could you be more explicit how you would do it with templates?

Alternatively we could use bitfields, which would make the code more readable without all those bit twiddling operations, and remove the need for the enum.

yes, bitfields could help. i've hesitated, but didn't use them because that would need some more steps for serialization (the "bit twiddling operations" are only limited to the two functions). In addition, how could that remove the need for the enum? you still need to tell it which bit is used for what.

As said in the inline comments, the scope of this patch is limited to improving the obvious drawbacks of the structure: several boolean used as u8. If people think that such a limited patch is not really needed and we should wait for a full rework of the structure, that's fine with me. We can just close that revision.

source/simulation2/components/CCmpRangeManager.cpp
196

yes, we can just assign all values InWorld=0, RetainInFog=1, ...

219

The scope of this patch was only to fix what was obviously wrong in the structure (using several u8 for booleans). Of course we can gain a bit here, but is it worth the hassle of having to decode such quantity each time we use it?

220

gaia is not included, so only 16 bits are currently used, but we should allow for extension (we should not make more difficult a future extension to 16 players if not really necessary)

221

same as line 210

222

same as line 211

223–224

agreed, even allowing for a future extension to 16 players needs 5 bits. But the question is do we really gain something compared to the hassle to have to decode the bits?

223–224

Could be, but once again, out of the scope as it would need some additionnal discussions to be sure how many entityFlagMasks are and will be needed in the future.

234

agreed

1702

:-) yep, i don't like computing useless quantities

leper added a comment.EditedJan 31 2017, 3:01 AM
In D101#3612, @mimo wrote:

Could you be more explicit how you would do it with templates?

Templating based on the used property, so the lookup (mask) is computed at compile time. So eg having HasProperty<RetainInFog> where that is either some type or some int.

If we'd like to go a bit over the top we could do somethin similar to LLVM's PackedVector, but that is more thinking along the lines of extending things for more than 8 (or 16 as is the case in most of this file) players. For the other packing opportunities taking some inspiration from their PointerIntPair (which can be nested) might at least be worth considering. That said I am aware that this is very unlikely to result in any benefit (for either readability or performance).

That said, bitfields seem like the nicest choice based on how the resulting code looks, and the generated code should be quite similar, otherwise I guess the bitfield code would be more likely to be optimized nicely.

Alternatively we could use bitfields, which would make the code more readable without all those bit twiddling operations, and remove the need for the enum.

yes, bitfields could help. i've hesitated, but didn't use them because that would need some more steps for serialization (the "bit twiddling operations" are only limited to the two functions). In addition, how could that remove the need for the enum? you still need to tell it which bit is used for what.

struct flags {
          uint InWorld : 1,
          uint RetainInFog : 1,
	​  uint RevealShore : 1,
	​  uint ScriptedVisibility : 1,
	​  uint SharedVision : 1
};
...

flags.InWorld = 1;
flags.RetainInFog = 0;

Serializing could be more complicated, but we can just memset that thing to 0, or = {0}; it, and be done with it. Then we can just serialize something of the right size (and of course assert (statically) that that thing has the right size. Seems more readable to me than the manual bit twiddling (which is fun, but most likely not needed here).

As said in the inline comments, the scope of this patch is limited to improving the obvious drawbacks of the structure: several boolean used as u8. If people think that such a limited patch is not really needed and we should wait for a full rework of the structure, that's fine with me. We can just close that revision.

Indeed, most of those things were just things I had (in more handwavy comments) in some TODO diff of mine. I do agree that just cleaning up the flags parameter is most likely already nice enough (though that 32 size most likely doesn't lead to strange cache-line overlaps, but meh). I suspect that there could be some performance gain (mostly by not having to iterate that much data) by packing that struct a bit more, though access patterns would have to be considered.

mimo updated this revision to Diff 440.Feb 3 2017, 8:30 PM

I've looked a bit at the different possibilities. bitfields seems to be really implementation dependent
and could lead to OOS if not done with enough care, bitset could be nice, but its minimum size in 8 bytes
on my 64bit system. So none are really satisfying.

Vulcan added a comment.Feb 3 2017, 9:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/305/ for more details.

[...], bitset could be nice, but its minimum size in 8 bytes
on my 64bit system. So none are really satisfying.

Bytes? Seems strange that there is no specialization for fewer bytes, given that at least x86 has instructions for those.

The code readability gained by using Has/SetFlag is nice!

source/simulation2/components/CCmpRangeManager.cpp
196

Probably add a None = 0x00, which might be useful in a few places below.

199

\n might help with the separation, and maybe a comment would help with showing that we have one unused bit here.

216

FlagsMask::Normal would be a bit clearer here.

226

A templated variant of this (and SetFlag) would be nice (the non-templated versions are only useful for the flags part. (And even that could be changed to do some if-else thing based on what the flag is).

template<int mask>
inline bool HasFlag() const { return (flags & mask) != 0; }

template<int mask>
inline void SetFlag(bool val) { flags = val ? (flags | mask) : (flags & ~mask); }

Which would be called by eg foo.HasFlag<FlagMasks::InWorld>(). Note that this doesn't work for the old flags code which can be solved by doing some if-else and providing the right thing statically, or just retaining the below functions too. Not sure if the tiny added complexity is worth it.

1543

This should be 0.

1555–1556

This is wrong, it should still be 0, as a querier should be able to look for multiple flag masks, and an entity should also be able to have a few of those (though this function should really only set one).

mimo updated this revision to Diff 458.Feb 5 2017, 12:30 PM

adding templated functions

mimo added a comment.Feb 5 2017, 12:44 PM
In D101#4150, @leper wrote:

[...], bitset could be nice, but its minimum size in 8 bytes
on my 64bit system. So none are really satisfying.

Bytes? Seems strange that there is no specialization for fewer bytes, given that at least x86 has instructions for those.

Certainly compiler dependent, but that's what gcc gives me: 8bytes in 64bit system and should be 4 bytes if 32bit. I agree it is strange, but i guess a performance issue.

source/simulation2/components/CCmpRangeManager.cpp
196

ok

199

ok

216

right

226

ok I've nonetheless kept the not templated version of SetFlag to avoid if-else which would dupplicate code between GetEntityFlagMask and SetEntityFlag

1555–1556

It was fine as long it is what GetEntityFlagMask returns when it does not find the identifier. But anyway, now switch to None

Vulcan added a comment.Feb 5 2017, 1:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/316/ for more details.

Looks fine from a quick glance, I'll have to test this later.

leper accepted this revision.Feb 24 2017, 4:11 PM

Work as expected. If someone is really bored they could benchmark what the difference in performance is on the huge combat demo map (I suspect that there'll be a slight improvement due to more entries fitting in the cache).

Maybe ref #3834 in the commit message.

source/simulation2/components/CCmpRangeManager.cpp
202

reserved for future use would be slightly nicer IMO.

1692

Can you add a \n before that too, since we're already sneaking in the below change?

This revision is now accepted and ready to land.Feb 24 2017, 4:11 PM
This revision was automatically updated to reflect the committed changes.