Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a BitSet for representing target types. #14943

Merged
merged 1 commit into from Mar 21, 2018

Conversation

Projects
None yet
4 participants
@RoosterDragon
Copy link
Member

RoosterDragon commented Mar 18, 2018

Use a BitSet rather than a HashSet<string> for representing target types. Since there are only a few distinct target types, we can easily represent these as a bitfield which is faster and more memory efficient than a HashSet.

Suggested by @chrisforbes: #14917 (comment), #14933 (comment).

In the default mods, there are the following numbers of distinct target types:

Mod Count
d2k 12
cnc 16
ts 17
ra 26

Currently we use a 32-bit integer to give us a limit of 32 - should this ever become insufficient it can always be upped to a 64-bit integer for a limit of 64.

Edit: Switched to System.Numerics.BigInteger - which means no limit!

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 18, 2018

I'm pretty confident that 32 bits won't be enough for third party mods, and I won't be surprised if some are even pushing past 64. What is the failure mode when this overflows? Does this throw an exception, or silently fail?

I'm okay with forcing a hard 64 limit provided that we throw a clear error when the limit is reached.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 18, 2018

Ping @GraionDilach who should be able to provide more insight from the modding community perspective.

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Mar 18, 2018

Errors, and providers a comment with suggestions for the unlucky modder who hits this problem & searches it (could always move the comment into the error message I suppose). Thrown here.

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Mar 18, 2018

If we're certain modders are likely to run into this a lot, we could try defining some supersized struct of sufficient size (e.g. 2 ulongs to provide 128 bits - and implement the bit fiddling needed).

Alternatively, we could add some complexity internally so it automagically falls back to an array or HashSet on surpassing 64-bits which would remove the limit.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 18, 2018

IMO making this 64 bit should be good enough for now and shouldn't introduce any meaningful perf overheads.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 19, 2018

I can't really support this.

Modders with complex mods will likely get thrown a lot on 64 I believe. Target type filtering is the most reliable and least error-prone way to filter actors for timed stat modifications and special effect/weapon immunities. Do note that RA2 just got a weapon-based mindcontrol and I also wired Black Lotus styled remote capturing to weapons for starters (because external capturing stunning the target is horribly retarded), and then chained weapons might also want spec targettype filtering to prevent them being wasted on the ore driller (something RA2 Prism did a lot). Just the sheer amount of traits using them now should have alerted the warning bells - infiltration is something classic RA already had twice (spy/thief).

In my own modding experience, relying on explicit templates/trait definitions limited to only the actors which should be affected by an effect might end up being too complicated to handle on the per-actor/per-template level if the templates can interact with each other/affect each other's traits (a simple example would be to have Temporal and EMP in the same mod, both disabling the unit, but Temporal also granting it weapon immunity for casual weapons, and this setup applied to stuff which can fly/be paradropped which involves another targettype switch, and again, this is just RA2 with EMP) and targettypes are a really good tool to be used along with global template filtering, to allow a better edge-case handling and less chance to crash out because of a messed-up cross-template interaction.

I can't tell how many target types I am using at this point though, but I'm pretty certain if I would theoretically take Mental Omega to OpenRA, it would take me a lot of headache to keep it within the 64 targettypes limit.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 19, 2018

@GraionDilach Modders with complex mods also love to hate on OpenRA's poor performance. A balance needs to be struck between capabilities and performance.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 19, 2018

If you are dismissing my criticism on sight, then why have you pinged me in the first place? I can understand that this PR might be able to give out a noticable perf increase due to the comment in #14933, but that alone does not justify a 64 targettype limit considering that this was never a problem before. Immunities should use targettypes as well meaningfully and not custom armors, and that leads to that a RA2/YR project wouldn't have much room to actually work with.

Target filtering was never that big of a deal of how you're acting now.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 19, 2018

I was hoping that you could give some indication as to what a reasonable limit would be for modders rather than dismissing the PR on sight because it implements a limit.

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Mar 19, 2018

System.Numerics.BigInteger would allow us to remove any limit - I'll find some time to see if it performs well - if so we can sidestep the "what arbitrarily low limit would still be acceptable" quandary.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 19, 2018

By the time I arrived, it seemed like 128 wouldn't be an option - something I'd be okay with personally. Going above 256 is definitely shouldn't be an aim, that's an amount of mess to be maintained properly, even in mind with additional future logics with the ability of employing targettype filtering as well.

@RoosterDragon RoosterDragon force-pushed the RoosterDragon:targets-bitset branch 2 times, most recently from 73698a8 to ad471b5 Mar 20, 2018

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Mar 20, 2018

BigInteger works well and means no limit! I've updated the PR to use it. This required adding a reference to System.Numerics.

@@ -41,7 +41,7 @@ SDK ?=
CSC = mcs $(SDK)
CSFLAGS = -nologo -warn:4 -codepage:utf8 -langversion:5 -unsafe -warnaserror
DEFINE = TRACE
COMMON_LIBS = System.dll System.Core.dll System.Data.dll System.Data.DataSetExtensions.dll System.Drawing.dll System.Xml.dll thirdparty/download/ICSharpCode.SharpZipLib.dll thirdparty/download/FuzzyLogicLibrary.dll thirdparty/download/MaxMind.Db.dll thirdparty/download/Eluant.dll thirdparty/download/rix0rrr.BeaconLib.dll
COMMON_LIBS = System.dll System.Core.dll System.Data.dll System.Data.DataSetExtensions.dll System.Drawing.dll System.Numerics.dll System.Xml.dll thirdparty/download/ICSharpCode.SharpZipLib.dll thirdparty/download/FuzzyLogicLibrary.dll thirdparty/download/MaxMind.Db.dll thirdparty/download/Eluant.dll thirdparty/download/rix0rrr.BeaconLib.dll

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 20, 2018

Author Member

Should I add System.Numerics here, or should I move it specifically to OpenRA.Game which is currently the only thing using it?

This comment has been minimized.

@pchote

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 20, 2018

Author Member

That already seems to include libmono-system-numerics4.0-cil surprisingly.

This comment has been minimized.

@pchote

pchote Mar 20, 2018

Member

huh, so it does.

@pchote
Copy link
Member

pchote left a comment

Code changes LGTM, just two naming nits and needs a rebase.

using System;
using System.Collections;
using System.Collections.Generic;
using BitSetSize = System.Numerics.BigInteger;

This comment has been minimized.

@pchote

pchote Mar 21, 2018

Member

IMO BitSetIndex would better reflect the way this is used.

/// <summary>
/// Indicates target types as defined on <see cref="Traits.ITargetable"/> are present in a <see cref="Primitives.BitSet{T}"/>.
/// </summary>
public sealed class TargetableType { TargetableType() { } }

This comment has been minimized.

@pchote

pchote Mar 21, 2018

Member

Can we rename this to TargetType to match previous conventions?

This comment has been minimized.

@RoosterDragon

RoosterDragon Mar 21, 2018

Author Member

Conflicts with the also reasonably named TargetType (i.e. the enum that tells you the type of a Target).

This comment has been minimized.

@pchote

pchote Mar 21, 2018

Member

Ah right, of course. I agree that TargetableType is our next best option.

Use a BitSet for representing target types.
- Rename Bits<T> to BitSet<T>.
- Implement set based helpers for BitSet<T>.
- When representing TargetTypes of ITargetable in various traits, use a BitSet<TargetableType> instead of HashSet<string> for better performance & reduced memory usage.
- Fix FieldLoader to trim input values when generating a BitSet<T>.
- Require T in BitSet<T> and BitSetAllocator<T> to be a class since it's just a marker value. This allows the JIT to instantiate generic code for these classes once, as they don't benefit from specialized code for T. (Typically JITs will generate shared code for all reference types, and unique code for every value type encountered).
@pchote

pchote approved these changes Mar 21, 2018

@abcdefg30 abcdefg30 merged commit 5bd5a38 into OpenRA:bleed Mar 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Mar 21, 2018

@RoosterDragon RoosterDragon deleted the RoosterDragon:targets-bitset branch Mar 21, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 23, 2018

@RoosterDragon Do we want to port the Mobile movement types and various other HashSet<string> type lists to use this new plumbing?

@RoosterDragon

This comment has been minimized.

Copy link
Member Author

RoosterDragon commented Mar 23, 2018

It's not vital from a performance perspective, however it would hardly be unwelcome. It's a bit better for memory, and also helps correctness since BitSet<T> can't be compared accidentally to BitSet<U> - whereas HashSet<string> can always be compared, even if they represent different types of stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.