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

Move various weapon stat-based effects into techs #935

Merged
merged 4 commits into from Feb 14, 2017

Conversation

Projects
None yet
5 participants
@geoffthemedio
Member

geoffthemedio commented Aug 30, 2016

Should have similar effect processing optimization results as moving species effects.

Updated to work for various weapons and upgrades, and handle case of unowned ships.

The idea is that instead of processing this effect as a separate single-target effectsgroup for each ship with the part, it can be processed once per empire.

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Aug 30, 2016

Member

I think "component:internal" is supposed to be used for additions/changes to the innards of the backend code which do not fit into the other "component" categories... so "component:scripting" should be completely sufficient here?

Member

Vezzra commented Aug 30, 2016

I think "component:internal" is supposed to be used for additions/changes to the innards of the backend code which do not fit into the other "component" categories... so "component:scripting" should be completely sufficient here?

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Aug 30, 2016

Member

just edit the labels; no need to post about it first.

Member

geoffthemedio commented Aug 30, 2016

just edit the labels; no need to post about it first.

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Aug 31, 2016

Member

Ok... it's just that I want to check back before I remove a label someone else set...

Member

Vezzra commented Aug 31, 2016

Ok... it's just that I want to check back before I remove a label someone else set...

@Vezzra Vezzra added this to the Next Release milestone Sep 23, 2016

@geoffthemedio geoffthemedio changed the title from Move mass driver stat-based effects into techs. Should have similar e… to Move various weapon stat-based effects into techs Nov 8, 2016

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 8, 2016

Member

Updated. Testing requested.

Member

geoffthemedio commented Nov 8, 2016

Updated. Testing requested.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 10, 2016

Contributor

Works for me.

Mass driver and laser damage scaled correctly when researching new levels.

Contributor

LGM-Doyle commented Nov 10, 2016

Works for me.

Mass driver and laser damage scaled correctly when researching new levels.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 10, 2016

Member

Do all monster parts still work? In particular, any non-player owned ships with SR_WEAPON_?_1 on them?

Would also be interesting if in a later-stage game with lots of player-owned ships, if there's any notable change in turn processing / meter updating time.

Member

geoffthemedio commented Nov 10, 2016

Do all monster parts still work? In particular, any non-player owned ships with SR_WEAPON_?_1 on them?

Would also be interesting if in a later-stage game with lots of player-owned ships, if there's any notable change in turn processing / meter updating time.

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Nov 10, 2016

Member

AFAIK, it shouldn't affect monsters at all, we specifically rewrote all the monster parts and hulls, including the guards, so they gave any bonuses directly and/or used unique monster stuff.

Going to test this now, curious why put it into Techs and not the parts themselves? A test for whether the owner has a tech is easy to script (and I'm testing a script using it now) but I don't know about the resource drain.

Member

MatGB commented Nov 10, 2016

AFAIK, it shouldn't affect monsters at all, we specifically rewrote all the monster parts and hulls, including the guards, so they gave any bonuses directly and/or used unique monster stuff.

Going to test this now, curious why put it into Techs and not the parts themselves? A test for whether the owner has a tech is easy to script (and I'm testing a script using it now) but I don't know about the resource drain.

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Nov 10, 2016

Member

OK, playing in the Fighters branch rebased, which AFAICS shouldn't affect things at all, and all my ranged weapon stats are reduced to zero, have hit end turn a couple times to see if it/s a load/update issue and it's not, even Flak Cannons are gone

Member

MatGB commented Nov 10, 2016

OK, playing in the Fighters branch rebased, which AFAICS shouldn't affect things at all, and all my ranged weapon stats are reduced to zero, have hit end turn a couple times to see if it/s a load/update issue and it's not, even Flak Cannons are gone

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 10, 2016

Member

The idea I'm basing this on is that if a weapon part has an effect, which only acts on the ship that has the part, then each ship has a separate effectsgroup to evaluate and execute, meaning for 200 ships with 5 parts, that's 1000 effectsgroups' activation conditions and scopes to evaluate (several of which end up being unnecessary due to stacking rules). If instead the effect is in a tech, then there's just one scope to evaluate. Even though single-ship Source scopes are trivial to evaluate, doing so ~1000 times involves a lot of creation and passing around of containers of pointers, as well as creating threads to execute the effects groups, probably some extra accounting stuff, etc. I suspect that doing a single effects group with 1000 targets once is a lot more efficient.

Member

geoffthemedio commented Nov 10, 2016

The idea I'm basing this on is that if a weapon part has an effect, which only acts on the ship that has the part, then each ship has a separate effectsgroup to evaluate and execute, meaning for 200 ships with 5 parts, that's 1000 effectsgroups' activation conditions and scopes to evaluate (several of which end up being unnecessary due to stacking rules). If instead the effect is in a tech, then there's just one scope to evaluate. Even though single-ship Source scopes are trivial to evaluate, doing so ~1000 times involves a lot of creation and passing around of containers of pointers, as well as creating threads to execute the effects groups, probably some extra accounting stuff, etc. I suspect that doing a single effects group with 1000 targets once is a lot more efficient.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 10, 2016

Member

@MatGB Did you rebuild from source first? There are important changes in ValueRef.cpp

Member

geoffthemedio commented Nov 10, 2016

@MatGB Did you rebuild from source first? There are important changes in ValueRef.cpp

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 10, 2016

Contributor

For monsters I fought Kraken and Snowflakes, which don't have SR_WEAPON_ in their descriptions.

I fought one Dyson forest and a Maintenance ship, which I think is SM_TREE and SM_GUARD_0, both with SR_WEAPON_1_1. I had basic shields and nothing unusual happened. This does not test the weapon tech upgrades.

I only played 100 turns, so I didn't noticed any change in lag.

So just a test of basic functionality.

Contributor

LGM-Doyle commented Nov 10, 2016

For monsters I fought Kraken and Snowflakes, which don't have SR_WEAPON_ in their descriptions.

I fought one Dyson forest and a Maintenance ship, which I think is SM_TREE and SM_GUARD_0, both with SR_WEAPON_1_1. I had basic shields and nothing unusual happened. This does not test the weapon tech upgrades.

I only played 100 turns, so I didn't noticed any change in lag.

So just a test of basic functionality.

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Nov 10, 2016

Member

Ah, I missed that there were backend changes, no, didn't recompile and am in the middle of work in Fighters so can't do that immediately, will do so when I'm done what I'm doing.

Member

MatGB commented Nov 10, 2016

Ah, I missed that there were backend changes, no, didn't recompile and am in the middle of work in Fighters so can't do that immediately, will do so when I'm done what I'm doing.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 28, 2016

Member

Bump!

Particularly if someone likes profiling, it would be nice to know if it makes a difference in games with lots of ships with lots of weapon parts.

Member

geoffthemedio commented Nov 28, 2016

Bump!

Particularly if someone likes profiling, it would be nice to know if it makes a difference in games with lots of ships with lots of weapon parts.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 29, 2016

Contributor

How did you profile it while you were developing it?

I usually find a problem and then write tests to measure the performance, so that I can see if things are improving or have improved.

I have not noticed this scripting issue as a problem in my playing or profiling so I have not drilled down into it. Where were you seeing an improvement and how big was it?

Contributor

LGM-Doyle commented Nov 29, 2016

How did you profile it while you were developing it?

I usually find a problem and then write tests to measure the performance, so that I can see if things are improving or have improved.

I have not noticed this scripting issue as a problem in my playing or profiling so I have not drilled down into it. Where were you seeing an improvement and how big was it?

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 29, 2016

Member

I didn't profile explicitly for this, but I recall there were previous cases where a similar change (the "moving species effects" mentioned in the first post) was made with substantial improvements, either noted qualitatively or through someone profiling / timing effect execution. I don't routinely play games long enough to have a relevant test case for this, though.

Member

geoffthemedio commented Nov 29, 2016

I didn't profile explicitly for this, but I recall there were previous cases where a similar change (the "moving species effects" mentioned in the first post) was made with substantial improvements, either noted qualitatively or through someone profiling / timing effect execution. I don't routinely play games long enough to have a relevant test case for this, though.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 29, 2016

Contributor

I assume that this PR, change the save state so you can't take a late game save, profile it, compile the new code and re-profile it. If that is the case then comparison is difficult.

The easy way would be to setup a test bench with just this code under test. Then you would know that it is input/output correct, performs the same function, and have performance information to determine which code was better. It is a targeted test. You can keep the code and add it to a test suite.

Another less satisfactory way is to run both sets of code in parallel through an entire game. Again you can compare both input/output and performance. This is how I tested/refactored the MapWnd supply lane coloring code. The test code is useless and will have to be tossed.

Right now option B is our only option.

Which effect processing loop do you expect to see a performance in?
How hard would it be to double up this code, so that the effect is processed twice, once by the old code and once by the new code?

Contributor

LGM-Doyle commented Nov 29, 2016

I assume that this PR, change the save state so you can't take a late game save, profile it, compile the new code and re-profile it. If that is the case then comparison is difficult.

The easy way would be to setup a test bench with just this code under test. Then you would know that it is input/output correct, performs the same function, and have performance information to determine which code was better. It is a targeted test. You can keep the code and add it to a test suite.

Another less satisfactory way is to run both sets of code in parallel through an entire game. Again you can compare both input/output and performance. This is how I tested/refactored the MapWnd supply lane coloring code. The test code is useless and will have to be tossed.

Right now option B is our only option.

Which effect processing loop do you expect to see a performance in?
How hard would it be to double up this code, so that the effect is processed twice, once by the old code and once by the new code?

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 29, 2016

Member

I don't think this would break a save.

Member

geoffthemedio commented Nov 29, 2016

I don't think this would break a save.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 30, 2016

Contributor

Good.

So the next question is where do you think is a good place to measure the performance change?

I can merge this PR with my latest long play and add some test code at that place. The improvement may be large, but if it is buried in the AI turn processing time, then I may miss it if I'm not looking in the correct location.

Contributor

LGM-Doyle commented Nov 30, 2016

Good.

So the next question is where do you think is a good place to measure the performance change?

I can merge this PR with my latest long play and add some test code at that place. The improvement may be large, but if it is buried in the AI turn processing time, then I may miss it if I'm not looking in the correct location.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Nov 30, 2016

Member

A game with lots of ships that have lots of weapon 1 to 4 parts in their designs should see the most benefit for effect updates... Basically Universe::UpdateMeterEstimatesImpl. Hopefully turn updates should go faster in particular, and perhaps focus changes or production queue manipulations.

Member

geoffthemedio commented Nov 30, 2016

A game with lots of ships that have lots of weapon 1 to 4 parts in their designs should see the most benefit for effect updates... Basically Universe::UpdateMeterEstimatesImpl. Hopefully turn updates should go faster in particular, and perhaps focus changes or production queue manipulations.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Nov 30, 2016

Contributor

I'll try profiling this and report back.

Contributor

LGM-Doyle commented Nov 30, 2016

I'll try profiling this and report back.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Dec 1, 2016

Contributor

Here are the times from profiling.

The most recent long game that I have saves for is based on the Fighters branch with the AI behavior and some of my sidepanel branches merged in. I added this PR for WeaponStatEffects (WSE). The profiling code was already there.

I played 3 turns, which include 2 battles, 1 invasion and some pathing. The bigger battle was 127 ships vs 18 ships, with 1.6K power of 69 Laser4 weapons with Solar Web vs 564 power of mixed Plasma and Laser4, with 162 fighter vs 107 fighters.

I only ran each case once and copied the results here.

InitTurn times in ms, with totals at the bottom.

Without WSE With WSE
4330 4452
4256 4152
4219 4021
4128 4360
--- ---
16933 16985

Universe::UpdateMeterEstimatesImpl times in ms.

I don't know for sure if each of these cases actually aligns. I moved the 296 ms number down two rows, because it obviously aligns with the 426 row since they are the only estimates with 94 targets.

Without WSE With WSE Number of targets
1109 1195 0
1228 1288 0
1095 1166 0
1529 1283 0
1047 1125 0
1376 1215 0
296 426 94
1089 1212 0
1157 1501 0
--- --- ---
9926 10411
Contributor

LGM-Doyle commented Dec 1, 2016

Here are the times from profiling.

The most recent long game that I have saves for is based on the Fighters branch with the AI behavior and some of my sidepanel branches merged in. I added this PR for WeaponStatEffects (WSE). The profiling code was already there.

I played 3 turns, which include 2 battles, 1 invasion and some pathing. The bigger battle was 127 ships vs 18 ships, with 1.6K power of 69 Laser4 weapons with Solar Web vs 564 power of mixed Plasma and Laser4, with 162 fighter vs 107 fighters.

I only ran each case once and copied the results here.

InitTurn times in ms, with totals at the bottom.

Without WSE With WSE
4330 4452
4256 4152
4219 4021
4128 4360
--- ---
16933 16985

Universe::UpdateMeterEstimatesImpl times in ms.

I don't know for sure if each of these cases actually aligns. I moved the 296 ms number down two rows, because it obviously aligns with the 426 row since they are the only estimates with 94 targets.

Without WSE With WSE Number of targets
1109 1195 0
1228 1288 0
1095 1166 0
1529 1283 0
1047 1125 0
1376 1215 0
296 426 94
1089 1212 0
1157 1501 0
--- --- ---
9926 10411
@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Dec 1, 2016

Member

Not much difference at that number of ships. I was thinking of somewhat larger fleets, though.

Member

geoffthemedio commented Dec 1, 2016

Not much difference at that number of ships. I was thinking of somewhat larger fleets, though.

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Dec 1, 2016

Contributor

That was the game I had sitting around.

However, the timers for InitTurn and UpdateMeterEstimatesImpl are currently in master. Anyone can merge this PR in a check the performance difference.

Contributor

LGM-Doyle commented Dec 1, 2016

That was the game I had sitting around.

However, the timers for InitTurn and UpdateMeterEstimatesImpl are currently in master. Anyone can merge this PR in a check the performance difference.

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Feb 12, 2017

Member

What's the status on this PR? Judging by the comments, it could be merged, OTOH it doesn't offer much improvement apparently. Any ideas what to do with it?

Member

Vezzra commented Feb 12, 2017

What's the status on this PR? Judging by the comments, it could be merged, OTOH it doesn't offer much improvement apparently. Any ideas what to do with it?

@Vezzra Vezzra modified the milestones: optional for v0.4.7, v0.4.7 Feb 12, 2017

@LGM-Doyle

This comment has been minimized.

Show comment
Hide comment
@LGM-Doyle

LGM-Doyle Feb 14, 2017

Contributor

It may/may not improve performance, but it does standardize the weapons effect in FOCS and that improves the code quality.

Contributor

LGM-Doyle commented Feb 14, 2017

It may/may not improve performance, but it does standardize the weapons effect in FOCS and that improves the code quality.

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Feb 14, 2017

Member

Would also be interesting to test on a much bigger game with lots more ships and weapon effects happening, such as one where someone reports getting "5 minute" turns ( http://freeorion.org/forum/viewtopic.php?f=25&t=10414 )

Member

geoffthemedio commented Feb 14, 2017

Would also be interesting to test on a much bigger game with lots more ships and weapon effects happening, such as one where someone reports getting "5 minute" turns ( http://freeorion.org/forum/viewtopic.php?f=25&t=10414 )

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Feb 14, 2017

Member

It may/may not improve performance, but it does standardize the weapons effect in FOCS and that improves the code quality.

I agree, and am in favor of merging.

Member

Vezzra commented Feb 14, 2017

It may/may not improve performance, but it does standardize the weapons effect in FOCS and that improves the code quality.

I agree, and am in favor of merging.

geoffthemedio added some commits Aug 30, 2016

Move mass driver stat-based effects into techs. Should have similar e…
…ffect processing optimization results as moving species effects.
-Moved some weapon effects into techs.
-Added separate effects for unowned ships weapon meters.
@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio Feb 14, 2017

Member

rebased

Member

geoffthemedio commented Feb 14, 2017

rebased

@geoffthemedio geoffthemedio merged commit ac97202 into master Feb 14, 2017

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@geoffthemedio geoffthemedio deleted the WeaponStatEffects branch Feb 14, 2017

@Vezzra Vezzra modified the milestones: v0.4.7, optional for v0.4.7 Feb 17, 2017

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Feb 18, 2017

Member

I can't follow the code well, but owned flak cannons aren't doing anything (showing strength 0 and not shooting), the unowned effect works and things like Sentries work as intended.

I think there's just a little thing missing but I can't figure out what.

Member

MatGB commented Feb 18, 2017

I can't follow the code well, but owned flak cannons aren't doing anything (showing strength 0 and not shooting), the unowned effect works and things like Sentries work as intended.

I think there's just a little thing missing but I can't figure out what.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Feb 18, 2017

Contributor

Try adding

[[WEAPON_BASE_EFFECTS(SR_WEAPON_0_1)]]

to the effectgroups of the SHP_ROOT_AGGRESSION tech (where the macro for SR_WEAPON_1_1 is as well).

Contributor

Morlic-fo commented Feb 18, 2017

Try adding

[[WEAPON_BASE_EFFECTS(SR_WEAPON_0_1)]]

to the effectgroups of the SHP_ROOT_AGGRESSION tech (where the macro for SR_WEAPON_1_1 is as well).

@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Feb 18, 2017

Member

While it absolutely makes sense, the game won't load and the logs say nothing, looks like it makes the tech invalid but the server crashes on me before the log gets written.

Member

MatGB commented Feb 18, 2017

While it absolutely makes sense, the game won't load and the logs say nothing, looks like it makes the tech invalid but the server crashes on me before the log gets written.

@Morlic-fo

This comment has been minimized.

Show comment
Hide comment
@Morlic-fo

Morlic-fo Feb 18, 2017

Contributor

Did you also add the relevant brackets required by syntax?

effectsgroups = [
    [[WEAPON_BASE_EFFECTS(SR_WEAPON_1_1)]]
    [[WEAPON_BASE_EFFECTS(SR_WEAPON_0_1)]]
]
Contributor

Morlic-fo commented Feb 18, 2017

Did you also add the relevant brackets required by syntax?

effectsgroups = [
    [[WEAPON_BASE_EFFECTS(SR_WEAPON_1_1)]]
    [[WEAPON_BASE_EFFECTS(SR_WEAPON_0_1)]]
]
@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB Feb 19, 2017

Member

D'oh! Committing fix now, thanks. (weirdly, the fix didn't start working until I built a new ship with the part, at which point all my existing ships got the upgrade)

Member

MatGB commented Feb 19, 2017

D'oh! Committing fix now, thanks. (weirdly, the fix didn't start working until I built a new ship with the part, at which point all my existing ships got the upgrade)

MatGB added a commit that referenced this pull request Feb 19, 2017

MatGB added a commit to MatGB/freeorion that referenced this pull request Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment