On 24.01.2017 05:16, Michael T. Pope wrote:
Hi,
>> Should 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.
> Also, 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.
> Nearly 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.
> Please 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.
> This 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
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Freecol-developers mailing list
Freecol-developers@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/freecol-developers
