Skip to content
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

Overhaul activateAddons #1051

Merged
merged 9 commits into from
Feb 15, 2019
Merged

Overhaul activateAddons #1051

merged 9 commits into from
Feb 15, 2019

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Feb 8, 2019

Cache the addons list at game start instead of always iterating through config at preInit.
Create a "sparse" list with only the addons that really need to be passed to activateAddons because activateAddons is VERY slow.

CBA common preInit is the 3rd slowest in my modset, and this code is the main cause.

Left of the red line you can see the iteration through config right after the preprocess and compile for the init functions. Red line is the call to activateAddons.

@commy2
Copy link
Contributor

commy2 commented Feb 8, 2019

"sparse"?

@commy2 commy2 added this to the 3.10 milestone Feb 8, 2019
@dedmen
Copy link
Contributor Author

dedmen commented Feb 8, 2019

https://en.wikipedia.org/wiki/Sparse_file. Got a better name?

Before:
tracy_2019-02-08_13-20-46
3,334 is the config lookup. And the empty space is the activate Addons.
314ms total for preinit
After:

tracy_2019-02-08_13-39-51
115ms for activate addons. 155ms total for preInit

Prestart:
tracy_2019-02-08_13-41-56

By filtering out addons with empty units we get from 3342 down to 1181 (with my modset)
CfgAddons stuff is apparently broken. whoops.

@commy2
Copy link
Contributor

commy2 commented Feb 8, 2019

Test with only CBA, CBA + ACE, and then with whatever you're using and compare them all.

@dedmen
Copy link
Contributor Author

dedmen commented Feb 8, 2019

tracy_2019-02-08_13-59-17
Fixed. Only 412 addons left if you filter out empty ones or already loaded ones.
That's 412 addons passed to activateAddons, instead of previously 3342.

common preinit down to 114ms for activateAddons and 137ms total time.

and compare them all

I have things to do you know?

One run takes like 5 minutes.

@dedmen dedmen changed the title WIP: Overhaul activateAddons Overhaul activateAddons Feb 8, 2019
@PabstMirror
Copy link
Contributor

ace uses activatedAddons command which dropped from count 1419 down to 1312

https://github.com/acemod/ACE3/blob/master/addons/common/scripts/checkVersionNumber.sqf#L10

We should probably just start using cba_common_addons, which should contain the original values

@commy2
Copy link
Contributor

commy2 commented Feb 8, 2019

activatedAddons definitely should not be used, because even some modules break it.
On the other hand, I am not sure we should also break it, just because of some milliseconds.

@jonpas
Copy link
Member

jonpas commented Feb 8, 2019

I have things to do you know?

So do most people... you are no special furry! 😆

@commy2
Copy link
Contributor

commy2 commented Feb 9, 2019

We badly need multiline suggestions.

@commy2 commy2 added WIP and removed Needs Testing labels Feb 9, 2019
Copy link
Contributor

@commy2 commy2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like everything, except the variable names. With my suggestions the script becomes more "poetic" I think.

Also needs toLower I'd say.

commy2 and others added 3 commits February 9, 2019 13:02
Co-Authored-By: dedmen <dedmen@users.noreply.github.com>
Co-Authored-By: dedmen <dedmen@users.noreply.github.com>
Co-Authored-By: dedmen <dedmen@users.noreply.github.com>
@commy2 commy2 added Cleanup and removed WIP labels Feb 15, 2019
@commy2 commy2 merged commit d231d94 into CBATeam:master Feb 15, 2019
@PabstMirror PabstMirror mentioned this pull request Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants