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

[master < T1168] Property lookup caching #1168

Merged
merged 52 commits into from
Sep 11, 2023
Merged

[master < T1168] Property lookup caching #1168

merged 52 commits into from
Sep 11, 2023

Conversation

antepusic
Copy link
Contributor

@antepusic antepusic commented Aug 15, 2023 •
edited
Loading

Evaluating expressions that contain many same-variable property lookups is slower than optimal because GetProperty has linear complexity. This PR attempts to solve that by preloading and caching all properties of the variables that have multiple property lookups; the properties are cached in the execution context.

Related to #1159 (SetProperty optimization).

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

Introduce property caching for multiple same-variable property lookups

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses

We’ve optimized queries that build maps with multiple same-variable property lookups.

  • Tag someone from docs team in the comments

antepusic and others added 27 commits July 26, 2023 15:41
@antepusic
Publish branch for collaboration
0b430c4
@antepusic
Add UpdateOnSetProperty optimization
eb31b7b
@Josipmrden
Merge branch 'master' into optimize-set-property
6ddd3b4
@antepusic
Optimize two-tier index lookup structure
e3c3fc3
@antepusic
Merge branch 'optimize-set-property' of https://github.com/memgraph/m…
48b77db 
…emgraph into optimize-set-property
@antepusic
Simplify code by iterating from properties
67664fe
@antepusic
Clean up code
0723cb9
@antepusic
Use a meaningful name for the index lookup map
6c6ea06
@antepusic
Merge branch 'master' into optimize-set-property
8e1f68e
@antepusic
Add optimized constraint check
258af5b
@antepusic
Merge branch 'optimize-set-property' of https://github.com/memgraph/m…
853ec4a 
…emgraph into optimize-set-property
@antepusic
Remove comments
cee9d40
@Josipmrden
Merge branch 'master' into optimize-set-property
aa80973
@antepusic
Add GetProperty optimization
2fdaff9
@antepusic
Merge branch 'optimize-set-property' of https://github.com/memgraph/m…
5f3c699 
…emgraph into optimize-set-property
@antepusic
Guarantee that GetProperty returns
d268aac
@antepusic
Fix bug on setting properties
6957753
@antepusic
Merge branch 'master' into optimize-set-property
455fac4
@antepusic
Undo GetProperty changes to VertexAccessor
f519bdc
@antepusic
Merge branch 'optimize-set-property' of https://github.com/memgraph/m…
9c0880d 
…emgraph into optimize-set-property
@antepusic
Add property caching in parent node (MapLiteral)
ad2824c
@antepusic
Tidy up code
45602be
@antepusic
Mark parameter as unused
6003b8b
@antepusic
Add null property handling
5218c32
@antepusic
Fix cloning of extended AST classes
f78cb54
@antepusic
Undo commits about SetProperty
0522908
@antepusic
Cache property lookups inside EvaluationContext
d2d5edd
@antepusic antepusic self-assigned this Aug 15, 2023
@antepusic antepusic marked this pull request as draft August 15, 2023 22:48
Josipmrden
Josipmrden requested changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@Josipmrden Josipmrden left a comment

Choose a reason for hiding this comment

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

Overall good job, but some files are present that should not be.

src/query/frontend/ast/ast.hpp Outdated Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
src/storage/v2/inmemory/label_property_index.cpp Outdated Show resolved Hide resolved
src/storage/v2/inmemory/label_property_index.hpp Outdated Show resolved Hide resolved
src/storage/v2/inmemory/unique_constraints.cpp Outdated Show resolved Hide resolved
src/storage/v2/inmemory/unique_constraints.hpp Outdated Show resolved Hide resolved
tests/gql_behave/tests/memgraph_V1/features/property_caching.feature Outdated Show resolved Hide resolved
andrejtonev
andrejtonev reviewed Aug 18, 2023
View reviewed changes
Copy link
Contributor

@andrejtonev andrejtonev left a comment

Choose a reason for hiding this comment

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

Looks good! 💪
Some C++ related comments.
Plus a few places we could maybe gain performance as well. Not sure about them, since I'm missing the big picture. If you want we could jump on a call Monday.

src/query/frontend/semantic/symbol_generator.cpp Outdated Show resolved Hide resolved
src/query/frontend/semantic/symbol_generator.cpp Outdated Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
src/query/interpret/eval.hpp Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
@antepusic antepusic linked an issue Aug 24, 2023 that may be closed by this pull request
SetProperties working poorly when there is a lot of indexes #909
Closed
@antepusic antepusic added the feature feature label Sep 6, 2023
andrejtonev
andrejtonev approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@andrejtonev andrejtonev left a comment

Choose a reason for hiding this comment

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

Great job!

src/query/interpret/eval.hpp Show resolved Hide resolved
src/query/interpret/eval.hpp Outdated Show resolved Hide resolved
@antepusic
Copy link
Contributor Author

@vpavicic Release note in the original comment

@antepusic antepusic added Ship it PR ready to be merged and removed Ready for review PR is ready for review labels Sep 11, 2023
@Josipmrden Josipmrden merged commit 29a505c into master Sep 11, 2023
6 checks passed
@Josipmrden Josipmrden deleted the property-lookup-caching branch September 11, 2023 11:03
@vpavicic vpavicic mentioned this pull request Sep 11, 2023
Merged
4 tasks
@gitbuda gitbuda removed the Ship it PR ready to be merged label Sep 11, 2023
@vpavicic vpavicic added the Docs - changelog only Docs - changelog only label Sep 12, 2023
as51340 pushed a commit that referenced this pull request Nov 10, 2023
@antepusic @as51340
Property lookup caching (#1168)
4ca8801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josipmrden Josipmrden Josipmrden approved these changes

@andrejtonev andrejtonev andrejtonev approved these changes

Assignees

@antepusic antepusic

Labels
Docs - changelog only Docs - changelog only feature feature
Projects
No open projects
memgraph-v2 issues
Status: ✅ Done
Milestone
mg-v2.11.0
Development

Successfully merging this pull request may close these issues.

SetProperties working poorly when there is a lot of indexes
5 participants
@antepusic @Josipmrden @andrejtonev @gitbuda @vpavicic