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

Optimize redundant logic used in queries #7061

Merged
merged 10 commits into from
Dec 16, 2020
Merged

Conversation

pdiaz
Copy link
Contributor

@pdiaz pdiaz commented Dec 12, 2020

After upgrading parse-server from 2.7.4 to 4.4.0, MongoDB 4.0 maxed CPU usage from what used to be barely 20% usage.

This does reduce the redundant complexity on queries that involve pointer permissions.

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #7061 (cf304d5) into master (f01059f) will increase coverage by 8.47%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7061      +/-   ##
==========================================
+ Coverage   85.41%   93.89%   +8.47%     
==========================================
  Files         169      169              
  Lines       12425    12468      +43     
==========================================
+ Hits        10613    11707    +1094     
+ Misses       1812      761    -1051     
Impacted Files Coverage Δ
src/Controllers/DatabaseController.js 95.61% <100.00%> (+1.54%) ⬆️
src/Controllers/SchemaController.js 97.32% <0.00%> (+0.38%) ⬆️
src/RestWrite.js 93.67% <0.00%> (+0.48%) ⬆️
src/GraphQL/loaders/defaultGraphQLTypes.js 97.06% <0.00%> (+0.73%) ⬆️
src/triggers.js 94.60% <0.00%> (+1.40%) ⬆️
src/ParseServerRESTController.js 98.36% <0.00%> (+1.63%) ⬆️
src/batch.js 92.30% <0.00%> (+1.92%) ⬆️
src/Controllers/FilesController.js 94.00% <0.00%> (+2.00%) ⬆️
src/Config.js 90.47% <0.00%> (+2.04%) ⬆️
... and 21 more

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 f01059f...cf304d5. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Dec 12, 2020

Thanks for the PR! Can you write a test case for this?

@mtrezza
Copy link
Member

mtrezza commented Dec 12, 2020

Would you please also open an issue for this to explain the proposed change? This allow others to join the discussion more easily and helps us to better trace changes in Parse Server.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Some minor comments

CHANGELOG.md Outdated Show resolved Hide resolved
src/Controllers/DatabaseController.js Outdated Show resolved Hide resolved
src/Controllers/DatabaseController.js Outdated Show resolved Hide resolved
src/Controllers/DatabaseController.js Outdated Show resolved Hide resolved
Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Just a few nits

src/Controllers/DatabaseController.js Show resolved Hide resolved
src/Controllers/DatabaseController.js Show resolved Hide resolved
@dplewis
Copy link
Member

dplewis commented Dec 14, 2020

Have you tried your branch in production or tested it against your large database?

@mtrezza
Copy link
Member

mtrezza commented Dec 14, 2020

Before merging, mind any ongoing discussion in #7065.

@pdiaz
Copy link
Contributor Author

pdiaz commented Dec 14, 2020

Have you tried your branch in production or tested it against your large database?

Yes, after this optimization performance went back to the same range as before.

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

LGTM! @mtrezza How does it look?

@mtrezza
Copy link
Member

mtrezza commented Dec 14, 2020

I see that the test cases test the new reduction methods of the database controller, but there is no high-level test case that tests whether these methods are implemented correctly within the controller and in which scenarios they are actually applied to a query. Can we add some test cases that make a ParseQuery and ensure the optimization of the resulting query?

@pdiaz
Copy link
Contributor Author

pdiaz commented Dec 14, 2020

I see that the test cases test the new reduction methods of the database controller, but there is no high-level test case that tests whether these methods are implemented correctly within the controller and in which scenarios they are actually applied to a query. Can we add some test cases that make a ParseQuery and ensure the optimization of the resulting query?

@mtrezza Good call! I previously tested only with two pointer permissions, so I added two extra test cases for 2 and 3 pointer permissions on a class and fixed and issue detected with the second case.

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Just trivia, but on a general note:

Does this optimization apply to all $or / $and across a query or only related pointer permissions?

spec/DatabaseController.spec.js Outdated Show resolved Hide resolved
@pdiaz
Copy link
Contributor Author

pdiaz commented Dec 14, 2020

Does this optimization apply to all $or / $and across a query or only related pointer permissions?

@mtrezza No all, only on find operations related to pointer permissions and not the queries themselves. Update operations could also have unoptimized $and operations.

A full optimizer for queries in general would require many more test cases.

@mtrezza
Copy link
Member

mtrezza commented Dec 14, 2020

A full optimizer for queries in general would require many more test cases.

I was thinking into that direction. A Parse Query Optimizer would be an interesting feature, but maybe that's more a job for the guys at MongoDB.

This PR looks good to me, ready for merge I'd say, just need to look into the failing tests.

@pdiaz
Copy link
Contributor Author

pdiaz commented Dec 14, 2020

A full optimizer for queries in general would require many more test cases.

I was thinking into that direction. A Parse Query Optimizer would be an interesting feature, but maybe that's more a job for the guys at MongoDB.

This PR looks good to me, ready for merge I'd say.

Leaving that task to MongoDB guys means that a MongoDB upgrade would be required for everybody and that would take even much longer. I've been already working on that idea in my own, it will take some time to be ready.

This PR right now is already a good step forward.

@pdiaz
Copy link
Contributor Author

pdiaz commented Dec 14, 2020

@mtrezza @dplewis Looks like PG tests are a bit flaky. Can you trigger a restart?

@mtrezza
Copy link
Member

mtrezza commented Dec 14, 2020

Checks passed. @dplewis any thoughts before merging?

@dplewis dplewis merged commit c46e8a5 into parse-community:master Dec 16, 2020
dplewis added a commit that referenced this pull request Feb 21, 2021
fix tests

Postgres Support

Update parse to 2.19.0 (#7060)

Fix Prettier (#7066)

Remove cache clear on validateObjects

Improve add class if not exist

Improve modifying schema instead of clearing

Improve enforce class exists

Fix flaky Test

Release 4.5.0 (#7070)

* Release 4.5.0

* Update CHANGELOG.md

Co-authored-by: Tom Fox <[email protected]>

* Improve braking change note

* Create a breaking changes sub-section

* Add release action

Co-authored-by: Tom Fox <[email protected]>

Improve issue templates & add PR template (#7051)

* improved feature suggestion template

* added test case chapter to bug report template

* PR wording

* added PR template

* improved formatting in issue template

* removed checkbox for concept due to new GH discussions process

* improved wording

* improved PR todo list

* amended PR checklist; minor rewording

* removed duplicate wording

* add securtiy check section to contribution guide

fix PR template file location (#7074)

Optimize redundant logic used in queries (#7061)

* Optimize redundant logic used in queries

* Added CHANGELOG

* Fixed comments and code style after recommendations.

* Fixed code style after recommendation.

* Improved explanation in comments

* Added tests to for logic optimizations

* Added two test cases more and some comments

* Added extra test cases and fixed issue found with them.

* Removed empty lines as requested.

Co-authored-by: Pedro Diaz <[email protected]>

FileUpload options for Server Config (#7071)

* New: fileUpload options to restrict file uploads

* review changes

* update review

* Update helper.js

* added complete fileUpload values for tests

* fixed config validation

* allow file upload only for authenicated user by default

* fixed inconsistent error messages

* consolidated and extended tests

* minor compacting

* removed irregular whitespace

* added changelog entry

* always allow file upload with master key

* fix lint

* removed fit

Co-authored-by: Manuel Trezza <[email protected]>

Fix: context for afterFind (#7078)

* Fix: context for afterFind

* Update CHANGELOG.md

Co-authored-by: Manuel <[email protected]>

Fix max listener warning from livequery server (#7083)

* fix max listner warning

* fix

* Clean test log

Run definitions

pg fix

fix: upgrade ws from 7.4.0 to 7.4.1 (#7098)

Snyk has created this PR to upgrade ws from 7.4.0 to 7.4.1.

See this package in npm:
https://www.npmjs.com/package/ws

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade ldapjs from 2.2.2 to 2.2.3 (#7095)

Snyk has created this PR to upgrade ldapjs from 2.2.2 to 2.2.3.

See this package in npm:
https://www.npmjs.com/package/ldapjs

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade semver from 7.3.2 to 7.3.4 (#7092)

Snyk has created this PR to upgrade semver from 7.3.2 to 7.3.4.

See this package in npm:
https://www.npmjs.com/package/semver

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr

fix: upgrade uuid from 8.3.1 to 8.3.2 (#7101)

Snyk has created this PR to upgrade uuid from 8.3.1 to 8.3.2.

See this package in npm:
https://www.npmjs.com/package/uuid

See this project in Snyk:
https://app.snyk.io/org/acinader/project/8c1a9edb-c8f5-4dc1-b221-4d6030a323eb?utm_source=github&utm_medium=upgrade-pr
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants