On 24.01.2017 05:16, Michael T. Pope wrote:
Hi,
Post by Michael T. PopePost by Enrico Weigelt, metux IT consultShould we perhaps consolidate all the ability handling inside
FeatureContainer and let FreeColObject just call it ?
I am not sure what you intend here that is different from the current
implementation.
Better performance. I've seen that abilities are checked a lot, and
the current implementation isn't particularily efficient. In
FeatureContainer I've already optimized hasAbility() to do direct
lookup in the set(s) and bail out as fast as possible (w/o going
through getAbilities()). The downside is that I've currently have
a bit code duplication between the single-set and all-sets case
(id==null vs id!=null).
Looking at Colony, we're essentially combining two sets here (the colony
itself and the owner) - in both an ability can be absent or disabled.
To optimize it in a similar way as I've done in FeatureContainer, as
well as reducing the code in FeatureContainer, I'm thinkng of moving
out the single-set test with a 3-state result (yes,no,missing), so
the individual checks can be easily chained together.
Post by Michael T. PopeAlso, I have reviewed the first 50 or so of your patches. I do not
understand your objection to Streams.
It is sloooow and memory intensive. Just look at the jdk source, what
happens behind the scenes. Their streams api is bloaty and slow by
design, they
And it requires jdk8, which I have on none of my production machines,
running stable/lts distros (and properly packaging that monster is a
magnitudes larger job).
In the long run, I'd also like to get it running w/ AOT (to get the
real performance boost), as well as Dalvik.
Post by Michael T. PopeNearly all the patches removing Streams use bloat the code.
Note quite, a few of them do (yes, eg. finding list elements by min or
max on some attribute could be written more elegant.
OTOH, eg. replacing
| if (any(buildableType.getRequiredAbilities().entrySet(),
| e -> e.getValue() != hasAbility(e.getKey()))) {
| return NoBuildReason.MISSING_ABILITY;
| }
by
| for (Entry<String,Boolean> e :
buildableType.getRequiredAbilities().entrySet())
| if (e.getValue() != hasAbility(e.getKey()))
| return NoBuildReason.MISSING_ABILITY;
Doesn't really bloat it up (except a few more keystrokes for writing
down the actual types - but then the human reader doesn't have to
guess anymore), but it skips the function object instance and virtual
invocations per element.
Or even this:
| forEachMapEntry(buildingType.getRequiredAbilities(),
| e -> appendRequiredAbility(doc, e.getKey(), e.getValue()));
vs this:
| for (Map.Entry<String,Boolean> e :
buildingType.getRequiredAbilities().entrySet())
| appendRequiredAbility(doc, e.getKey(), e.getValue());
The old version is just a few chars longer (and more expressive, as it
actually shows the types involved), but way more expensive.
Post by Michael T. PopePlease do not do that *unless* you can
demonstrate a real benefit, and I think you will have to look hard if you
think that performance will significantly improved. I checked my
test results, and over the period from mid-2015 to early 2016 when I
added most of the Streams code, the average turn speed of my overnight
tests (all-AI simulations with no waiting for interactive users) decreased
by about 2%, which is within the bounds of error.
Maybe you've just replaced even more inefficient code. On my site, my
branch ontop of mainline is notably faster than the version coming on
precise -- *without* any streams, on jre7.
Post by Michael T. PopeThis is obviously not
definitive as a lot of other code went in at the same time, but certainly
suggestive that Streams have negligible impact, and even if Streams are
fully to blame for the change, I am *not* worried that it now takes 7 AI
players 5.2s to move rather than 5.1s.
On my site it's magnitudes smaller. Haven't looked at the whole AI area
yet, my next 2do is the startup, which still is uncomfortably slow ...
--mtx