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

Fix issues with PerFrameHandlers #236

Merged
merged 4 commits into from Jan 12, 2016

Conversation

Projects
None yet
5 participants
@commy2
Contributor

commy2 commented Jan 10, 2016

ref: #229, #230

  • makes CBA_fnc_addPerFrameHandler and CBA_fnc_removePerFrameHandler safe to be used in scheduled environment.
  • moves stuff around in FUNC(onFrame) to reduce overhead
  • adds debug log when more than 1kk PFHs were added per mission.
  • adds PFH save game compatibility

I'm currently on 1.55 dev and there seems to be no array size limit, so I can't really test #229.

@commy2 commy2 self-assigned this Jan 10, 2016

@jokoho48

This comment has been minimized.

Show comment
Hide comment
@jokoho48

jokoho48 Jan 11, 2016

Member

@commy2 why you remove the clean up of the perFrameHandlerArray again? that remove every performance improvement i did?

Member

jokoho48 commented Jan 11, 2016

@commy2 why you remove the clean up of the perFrameHandlerArray again? that remove every performance improvement i did?

@commy2

This comment has been minimized.

Show comment
Hide comment
@commy2

commy2 Jan 11, 2016

Contributor

Can you explain what you mean? There are no checks added in this PR, only stuff removed or moved around.

Contributor

commy2 commented Jan 11, 2016

Can you explain what you mean? There are no checks added in this PR, only stuff removed or moved around.

@jokoho48

This comment has been minimized.

Show comment
Hide comment
@jokoho48

jokoho48 Jan 11, 2016

Member

you remove https://github.com/CBATeam/CBA_A3/pull/236/files#diff-173548a824c3a52bc2c10f270fd8dc72L44 this completly and that cleanup the perFrameHandlerArray to improve performance and remove nil from this perFrameHandlerArray array so that dont run throw the Basic Frame handler

Member

jokoho48 commented Jan 11, 2016

you remove https://github.com/CBATeam/CBA_A3/pull/236/files#diff-173548a824c3a52bc2c10f270fd8dc72L44 this completly and that cleanup the perFrameHandlerArray to improve performance and remove nil from this perFrameHandlerArray array so that dont run throw the Basic Frame handler

@commy2

This comment has been minimized.

Show comment
Hide comment
@commy2

commy2 Jan 11, 2016

Contributor

There are no nil entries in GVAR(perFrameHandlerArray), so this isNil is redundant.

Try this:

count cba_common_perFrameHandlerArray
-> 3

0 call CBA_fnc_removePerFrameHandler

count cba_common_perFrameHandlerArray
-> 2
Contributor

commy2 commented Jan 11, 2016

There are no nil entries in GVAR(perFrameHandlerArray), so this isNil is redundant.

Try this:

count cba_common_perFrameHandlerArray
-> 3

0 call CBA_fnc_removePerFrameHandler

count cba_common_perFrameHandlerArray
-> 2
@jokoho48

This comment has been minimized.

Show comment
Hide comment
@jokoho48

jokoho48 Jan 11, 2016

Member

ok now i see what you did 👍

Member

jokoho48 commented Jan 11, 2016

ok now i see what you did 👍

@commy2

This comment has been minimized.

Show comment
Hide comment
@commy2

commy2 Jan 11, 2016

Contributor

Yeah. You might have been thinking of GVAR(PFHhandles).

cba_common_PFHhandles
-> [0,1,2]

1 call CBA_fnc_removePerFrameHandler

cba_common_PFHhandles
-> [0,any,1]
Contributor

commy2 commented Jan 11, 2016

Yeah. You might have been thinking of GVAR(PFHhandles).

cba_common_PFHhandles
-> [0,1,2]

1 call CBA_fnc_removePerFrameHandler

cba_common_PFHhandles
-> [0,any,1]
{
_x params ["", "", "", "", "", "_handle"];
GVAR(PFHhandles) set [_handle, _forEachIndex];
} forEach GVAR(perFrameHandlerArray);

This comment has been minimized.

@PabstMirror

PabstMirror Jan 11, 2016

Contributor

This forEach loop needs to be above line 32 or it will fail on 2nd call to :

x_A = [{      diag_log text format ["---%1 - A Running %2", diag_tickTime, (_this select 1)];  }, 5, []] call CBA_fnc_addPerFrameHandler;
x_B = [{      diag_log text format ["---%1 - B Running %2", diag_tickTime, (_this select 1)];  }, 5, []] call CBA_fnc_addPerFrameHandler;
[x_A] call CBA_fnc_removePerFrameHandler;
[x_B] call CBA_fnc_removePerFrameHandler;  
@PabstMirror

PabstMirror Jan 11, 2016

Contributor

This forEach loop needs to be above line 32 or it will fail on 2nd call to :

x_A = [{      diag_log text format ["---%1 - A Running %2", diag_tickTime, (_this select 1)];  }, 5, []] call CBA_fnc_addPerFrameHandler;
x_B = [{      diag_log text format ["---%1 - B Running %2", diag_tickTime, (_this select 1)];  }, 5, []] call CBA_fnc_addPerFrameHandler;
[x_A] call CBA_fnc_removePerFrameHandler;
[x_B] call CBA_fnc_removePerFrameHandler;  

This comment has been minimized.

@PabstMirror

PabstMirror Jan 11, 2016

Contributor

All we really need is to find index of the top level _handle in the perFrameHandlerArray array, so, we could add an exit to the loop and skip the set I think.

@PabstMirror

PabstMirror Jan 11, 2016

Contributor

All we really need is to find index of the top level _handle in the perFrameHandlerArray array, so, we could add an exit to the loop and skip the set I think.

This comment has been minimized.

@commy2

commy2 Jan 11, 2016

Contributor

Using a find would be slow when GVAR(PFHhandles) get's big. With this method you only have to iterate through the PFHs that are still active. No idea which one is better. This is what already was there.

@commy2

commy2 Jan 11, 2016

Contributor

Using a find would be slow when GVAR(PFHhandles) get's big. With this method you only have to iterate through the PFHs that are still active. No idea which one is better. This is what already was there.

This comment has been minimized.

@commy2

commy2 Jan 11, 2016

Contributor

Derp. The problem is that the public handle is pushed to GVAR(PFHhandles) (which is for the internal handles, those who can change).
Thats why removing the PFH twice "works". The loop auto corrects the internal handle...

@commy2

commy2 Jan 11, 2016

Contributor

Derp. The problem is that the public handle is pushed to GVAR(PFHhandles) (which is for the internal handles, those who can change).
Thats why removing the PFH twice "works". The loop auto corrects the internal handle...

@jokoho48

This comment has been minimized.

Show comment
Hide comment
@jokoho48

jokoho48 Jan 11, 2016

Member

i knew something look wrong hmm

Member

jokoho48 commented Jan 11, 2016

i knew something look wrong hmm

@commy2

This comment has been minimized.

Show comment
Hide comment
@commy2

commy2 Jan 11, 2016

Contributor

Should be fixed, @PabstMirror

Contributor

commy2 commented Jan 11, 2016

Should be fixed, @PabstMirror

@commy2 commy2 changed the title from [WIP] Fix issues with PerFrameHandlers to Fix issues with PerFrameHandlers Jan 12, 2016

@commy2 commy2 removed the WIP label Jan 12, 2016

@commy2

This comment has been minimized.

Show comment
Hide comment
@commy2

commy2 Jan 12, 2016

Contributor

I think we should use the debug in this PR to figure out what PFH is added more than a million times in missions running longer than 4 hours. Maybe we can fix that (if it's a PFH in ACE) and we don't need to change the handles at all.

Contributor

commy2 commented Jan 12, 2016

I think we should use the debug in this PR to figure out what PFH is added more than a million times in missions running longer than 4 hours. Maybe we can fix that (if it's a PFH in ACE) and we don't need to change the handles at all.

@Killswitch00

This comment has been minimized.

Show comment
Hide comment
@Killswitch00

Killswitch00 Jan 12, 2016

Contributor

I agree - the debug printout should be useful in identifying the PFH overruns

Contributor

Killswitch00 commented Jan 12, 2016

I agree - the debug printout should be useful in identifying the PFH overruns

Killswitch00 added a commit that referenced this pull request Jan 12, 2016

Merge pull request #236 from CBATeam/fixPFHscheduled
Fix issues with PerFrameHandlers

@Killswitch00 Killswitch00 merged commit a39aeab into master Jan 12, 2016

@ViperMaul ViperMaul added this to the 2.2.1 milestone Jan 16, 2016

@thojkooi thojkooi deleted the fixPFHscheduled branch Apr 25, 2016

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