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

Fix #5337 - suppress rewrite config in db without changes #5367

Merged
merged 3 commits into from Mar 2, 2020

Conversation

MurzNN
Copy link
Contributor

@MurzNN MurzNN commented Feb 29, 2020

Now Strapi rewrite all 'plugin_content_manager_configuration_content_types' keys in "core_store" database table on each restart.
I add comparison of stored config with generated, and skip unnecessary write config to database, when there are no changes.
fixes #5367

Signed-off-by murznn@gmail.com

@codecov
Copy link

codecov bot commented Feb 29, 2020

Codecov Report

Merging #5367 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5367   +/-   ##
=======================================
  Coverage   13.71%   13.71%           
=======================================
  Files         633      633           
  Lines        8685     8685           
  Branches     1337     1337           
=======================================
  Hits         1191     1191           
  Misses       6319     6319           
  Partials     1175     1175           
Flag Coverage Δ
#front 11.67% <ø> (ø) ⬆️
#unit 38.42% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e7689c...cb71cc5. Read the comment docs.

@derrickmehaffy
Copy link
Member

@MurzNN your commits aren't passing the DCO check, can you please fix this? (You need to sign each commit)

Now Strapi rewrite all 'plugin_content_manager_configuration_content_types' keys in "core_store" database table on each restart.
I add comparison of stored config with generated, and skip unnecessary write config to database, when there are no changes.

Signed-off-by: Alexey Murz Korepov <murznn@gmail.com>
@MurzNN
Copy link
Contributor Author

MurzNN commented Feb 29, 2020

@derrickmehaffy, thanks for tip, I fix this.

key: configurationKey(key),
value: config,
});
if(JSON.stringify(currentConfig) != JSON.stringify(storedConfig)) {
Copy link
Member

Choose a reason for hiding this comment

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

Using stringify isn't an efficient way to compare the object. lodash isEqual would be faster for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for tip, I replace JSON.stringify to lodash isEqual

@derrickmehaffy
Copy link
Member

Without the changes that @alexandrebodin mentioned, I tested this against my own project and it dropped the startup time from around 4200ms to about 3600ms so there is certainly gains to be made here. Using the lodash method would also likely drop this even more.

@@ -37,13 +37,13 @@ const setModelConfiguration = async (key, value) => {
}
});

if(JSON.stringify(currentConfig) != JSON.stringify(storedConfig)) {
if(_.isEqual(currentConfig, storedConfig) == false) {
Copy link
Member

Choose a reason for hiding this comment

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

can you please use !_.isEqual(a,b)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@alexandrebodin
Copy link
Member

@derrickmehaffy would you mind testing your perfs with the new commit ?

Signed-off-by: Alexey Murz Korepov <murznn@gmail.com>
Signed-off-by: Alexey Murz Korepov <murznn@gmail.com>
@derrickmehaffy
Copy link
Member

@derrickmehaffy would you mind testing your perfs with the new commit ?

Yup can do, I'll test right now and edit with my findings

@derrickmehaffy
Copy link
Member

derrickmehaffy commented Mar 2, 2020

So this is actually about 200ms slower on average compared to previously.

No Changes: startup in 4600
Previous Changes: startup in 3600
Current (lodash): 3800 to 3900

And I have not modified my schema at all here.

Edit for reference:
I have 98 Models in /api with 2 custom model folders (nothing in the database here) and no extra plugins minus documentation and GraphQL which shouldn't matter.

So all in all this should be saving me 98 writes to the database, and just comparing the ctm schemas. I'll some more logging to determine what queries it is actually running.

@alexandrebodin alexandrebodin added the issue: bug Issue reporting a bug label Mar 2, 2020
@alexandrebodin alexandrebodin removed the request for review from soupette March 2, 2020 16:08
@alexandrebodin alexandrebodin added the issue: enhancement Issue suggesting an enhancement to an existing feature label Mar 2, 2020
@alexandrebodin alexandrebodin added this to the 3.0.0-beta.19 milestone Mar 2, 2020
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM

@alexandrebodin
Copy link
Member

Thanks for your PR sir :D

@alexandrebodin alexandrebodin merged commit 01115c0 into strapi:master Mar 2, 2020
@alexandrebodin alexandrebodin added source: core:content-manager Source is core/content-manager package and removed issue: bug Issue reporting a bug labels Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants