widelands

Merge lp://staging/~widelands-dev/widelands/bug-1786613-500ms-return-skipped into lp://staging/widelands

  1. bug-1786613-500ms-return-skipped
  2. Merge into trunk
Proposed by Toni Förster
Status: Superseded
Proposed branch: lp://staging/~widelands-dev/widelands/bug-1786613-500ms-return-skipped
Merge into: lp://staging/widelands
Diff against target: 12 lines (+1/-1)
1 file modified
src/logic/map_objects/tribes/productionsite.cc (+1/-1)
To merge this branch: bzr merge lp://staging/~widelands-dev/widelands/bug-1786613-500ms-return-skipped
Reviewer Review Type Date Requested Status
hessenfarmer Approve
Review via email: mp+353028@code.staging.launchpad.net

This proposal supersedes a proposal from 2018-08-13.

This proposal has been superseded by a proposal from 2018-08-20.

Commit message

reduce waiting time to 500ms for skipped programs

Description of the change

That "penalty" is reduced to 500ms. Which, according to @hessenfarmer's testing may be the sweetspot

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote : Posted in a previous version of this proposal

Continuous integration builds have changed state:

Travis build 3782. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/415390227.
Appveyor build 3581. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786613_10s_return_skipped-3581.

Revision history for this message
hessenfarmer (stephan-lutz) wrote : Posted in a previous version of this proposal

Ok I tested this with different timings.
My machine is a Core 2 Duo T7500 @ 2,2 Ghz with 4 GB RAM.
My setup was
map: the nile
8 AI players 2 of each tribe
Let the game run for 4:50 hours
saved and loaded with every config.

results were:
original timing: around 30% cpu usage in task manager, peaks up to 45%
10 ms timing: around 33% with more and higher peaks up to 60%
100 ms timing: around 32% less peaks than 10s up to 50%
500 ms timing: around 30 to 31 % peaks up to 50% but not much frequent.

So I would propose to have a value of 500 to 1000ms as compromise to ensure minimal effect on performance.

review: Needs Fixing
Revision history for this message
hessenfarmer (stephan-lutz) wrote :

Looks good from my side now.
Would be good though if somebody with an underpowered machine would confirm minimal effect on performance

review: Approve
Revision history for this message
Toni Förster (stonerl) wrote :

> Looks good from my side now.
> Would be good though if somebody with an underpowered machine would confirm
> minimal effect on performance

Do you think it gets much more underpowered. Your processor is basically the same as mine. Just a few MHz slower. The only difference is RAM.

Revision history for this message
ypopezios (ypopezios) wrote :

In principle, if I was given a range to compromise with, I would go the conservative route and pick the edge of the range which is closer to the previous value. In this case, the given range is 500 to 1000ms, the previous value is 10000ms, so I would go with 1000ms.

Having said that, in my eyes an arbitrary value gets replaced by another arbitrary value. More tests of that kind won't make that value any less arbitrary. Therefore, I would comment this code for redesign in a future build.

Revision history for this message
hessenfarmer (stephan-lutz) wrote :

@ypopezios: In principle you are right. It is somewhat arbitrary or you might say it is try and error. On the other hand I don't know how to handle this in a different way, cause more frequent calls of the working programs will always consume more processing power. so the value of 500 to 1000ms was chosen due to the consumed processing power on my machine while knowing the kind of measurement had some big uncertainties. Therefore I asked to test his issue on an even more underpowered system. For my understanding the more underpowered a testsystem would be the more visible would be the effect of this change.
@Toni Förster:
I was thinking to have this tested on a netbook or similar. As we support 1024x600 resolution we should support the processing power of those configs as well.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 3791. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/415624736.
Appveyor build 3590. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1786613_500ms_return_skipped-3590.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: