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.

Sign up for GitHub

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

Jump to bottom

Revert string centralization #38321

Merged
merged 28 commits into from
Feb 25, 2020
Merged

Revert string centralization #38321

merged 28 commits into from
Feb 25, 2020

Conversation

kevingranade
Copy link
Member

Summary

SUMMARY: None

Purpose of change

Build times have been growing recently, upon investigation the recent move to centralize strings in src/cata_string_consts.h has been determined to be the culprit.

Describe the solution

Reverts the string centralization commits back to ones where conflicts start piling up.

Describe alternatives you've considered

We could revert either more (with more work) or fewer commits (generating work for later).
This is the series of reverts that apply cleanly without breaking the build, reverting commits beyond these works but build errors start creeping in.
We could also revert these piecemeal, possibly with as few as one original commit per revert PR. I'm not sure where the right tradeoff there is, because while smaller PRs are obviously better, delaying the reverts and spreading them out over time raise more chances of conflicts.

Testing

Everything builds and tests cleanly.
Game launches normally with no error and basic interactivity is present.
Build times are greatly improved.

Additional context

These are the results of timing tests I've run on an EC2 c4.2xlarge host I usually build on.
585fbbe (was master HEAD at the time)
real 9m20.844s
user 68m22.061s
sys 2m48.345s

d1b2f33 (was master HEAD at the time, effectively a retest to check jitter)
real 9m20.233s
user 68m0.841s
sys 2m46.542s

After reverting commits up to most recent merge
real 8m47.588s
user 63m45.326s
sys 2m43.966s

Rebuild of revert up to most recent merge.
real 8m47.384s
user 63m37.103s
sys 2m44.131s

Before creation of file (checked out the tree before the first PR in the series)
real 7m36.129s
user 54m24.264s
sys 2m30.595s

After reverts in current PR
real 8m6.127s
user 58m31.233s
sys 2m28.755s

Rebuild
real 8m6.016s
user 58m30.913s
sys 2m29.635s

@kevingranade
Revert "Moved more string consts into single file (CleverRaven#38237)"
ec97223 
This reverts commit 4987742.
@kevingranade
Revert "Moved more string consts into single file (CleverRaven#38220)"
0274847 
This reverts commit d9c6079.
@kevingranade
Revert "Move string consts into single file (CleverRaven#38195)"
f7b0acc 
This reverts commit b1b48ba.
@kevingranade
Revert "More string consts into single file (CleverRaven#38189)"
81ae825 
This reverts commit 4fffd96.
@kevingranade
Revert "More string consts into single file (CleverRaven#38183)"
d92ac3f 
This reverts commit 05940bf.
@kevingranade
Revert "Move string consts into single file (CleverRaven#38184)"
422740f 
This reverts commit 19c5698.
@kevingranade
Revert "Move string consts into single file (CleverRaven#38181)"
486c85c 
This reverts commit 55cabb7.
@kevingranade
Revert "Move more string consts into single file (CleverRaven#38175)"
6a76a89 
This reverts commit 571efb2.
@kevingranade
Revert "Merge pull request CleverRaven#38176 from Ramza13/string_cons…
2999a85 
…ts_and_more"

This reverts commit 9c9835f, reversing
changes made to 07995cd.
@kevingranade
Revert "Move more string consts into single file (CleverRaven#38169)"
1a86712 
This reverts commit 07995cd.
@kevingranade
Revert "Merge pull request CleverRaven#38168 from Ramza13/even_more_s…
a565702 
…tring_consts"

This reverts commit 2bf1c1e, reversing
changes made to 24a32c0.
@kevingranade
Revert "Merge pull request CleverRaven#38141 from ZhilkinSerg/sa-2020…
be68b0c 
…-02-18"

This reverts commit 142295b, reversing
changes made to f98c67d.
@kevingranade
Revert "Move string consts into single file (CleverRaven#38158)"
9c7e8ff 
This reverts commit 50059c8.
@kevingranade
Revert "Merge pull request CleverRaven#38157 from Ramza13/string_cons…
f2be6b5 
…ts_more_flags"

This reverts commit 8ec8e78, reversing
changes made to 72ef022.
@kevingranade
Revert "Move string_consts"
93a9dd0 
This reverts commit 0a6bf67.
@kevingranade
Revert "Merge pull request CleverRaven#38146 from Ramza13/battery"
3d793dc 
This reverts commit 9286722, reversing
changes made to e9f1cd7.
@kevingranade
Revert "Merge pull request CleverRaven#38134 from AMurkin/rename-blob"
c546b86 
This reverts commit 06f72d2, reversing
changes made to bcfe074.
@kevingranade
Revert "Yet more string consts moved out (CleverRaven#38130)"
7756f5b 
This reverts commit b5f18f3.
@kevingranade
Revert "Merge pull request CleverRaven#38115 from Ramza13/string_cons…
01a6a74 
…ts_game"

This reverts commit 9807657, reversing
changes made to 036465f.
@kevingranade kevingranade added Code: Build Issues regarding different builds and build environments [C++] Changes (can be) made in C++. Previously named `Code` labels Feb 25, 2020
@kevingranade kevingranade requested a review from a user February 25, 2020 04:16
@kevingranade
Copy link
Member Author

Side note, I've manually reviewed the whole PR and there's nothing here but swapping things back to literal strings and related changes. I HAD pulled in some extraneous stuff but that's all cleaned up now.

@ZhilkinSerg ZhilkinSerg merged commit 5fa9146 into CleverRaven:master Feb 25, 2020
@weekly-digest weekly-digest bot mentioned this pull request Mar 1, 2020
Closed
@kevingranade kevingranade deleted the revert-string-centralization branch June 28, 2020 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KorGgenT KorGgenT Awaiting requested review from KorGgenT

Assignees
No one assigned
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevingranade @ZhilkinSerg