Skip to content

Query module improvements#2176

Merged
Ignition merged 21 commits intomasterfrom
qm-improvements
Jul 19, 2024
Merged

Query module improvements#2176
Ignition merged 21 commits intomasterfrom
qm-improvements

Conversation

@imilinovic
Copy link
Contributor

@imilinovic imilinovic commented Jul 4, 2024 •
edited by Ignition
Loading

Performance enhancements:

  • C++ API MemoryDispatcherGuard had an expensive registration and fetch mechanism, replaced with thread_local state
  • Argument construction made temporary buffers unnecessarily
  • Hot-loading required module lookup on every call, we modified implementation to keep module lifetime for the whole function usage

@imilinovic imilinovic self-assigned this Jul 4, 2024
@imilinovic imilinovic added Docs - changelog only Docs - changelog only and removed draft labels Jul 4, 2024
@imilinovic imilinovic added this to the mg-v2.19.0 milestone Jul 4, 2024
@Ignition Ignition force-pushed the qm-improvements branch 2 times, most recently from 16aeaf8 to 7691f0c Compare July 8, 2024 08:10
antoniofilipovic
antoniofilipovic reviewed Jul 9, 2024
View reviewed changes
@Ignition Ignition self-assigned this Jul 12, 2024
@Ignition
Copy link
Contributor

Ignition commented Jul 12, 2024 •
edited by imilinovic
Loading

Tracking

Standard development

CI Testing Labels

  • Select the appropriate CI test labels (CI -build=build-name -test=test-suite)

Documentation checklist

  • Add the documentation label
  • Add the bug / feature label
  • Add the milestone for which this feature is intended
    • If not known, set for a later milestone
  • Write a release note, including added/changed clauses
    • C and C++ query modules are a lot faster now and no longer slow down when ran concurrently
  • [ Documentation PR link Reloading query modules docs documentation#916 ]
    • Is back linked to this development PR
  • @kgolubic

@Ignition Ignition added feature feature bug bug and removed feature feature labels Jul 12, 2024
@Ignition Ignition changed the title Slow QM improvements Query module improvements Jul 12, 2024
@Ignition Ignition marked this pull request as ready for review July 12, 2024 15:04
antoniofilipovic
antoniofilipovic suggested changes Jul 13, 2024
View reviewed changes
Copy link
Contributor

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Few comments, but very nice

@imilinovic imilinovic linked an issue Jul 15, 2024 that may be closed by this pull request
Query modules in C++ are being congested by the unique lock #1942
Closed
@imilinovic imilinovic added CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=core Run coverage build and core tests on push CI -build=jepsen -test=core Run jepsen build and core tests on push CI -build=debug -test=core Run debug build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push CI -build=release -test=stress Run release build and stress tests on push labels Jul 15, 2024
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 19, 2024

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

MarkoBarisic
MarkoBarisic approved these changes Jul 19, 2024
View reviewed changes
Copy link
Contributor

@MarkoBarisic MarkoBarisic left a comment

Choose a reason for hiding this comment

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

LGTM

@Ignition Ignition added this pull request to the merge queue Jul 19, 2024
Merged via the queue into master with commit 86ef2d0 Jul 19, 2024
@Ignition Ignition deleted the qm-improvements branch July 19, 2024 12:28
@Ignition Ignition removed CI -build=community -test=core Run community build and core tests on push CI -build=coverage -test=core Run coverage build and core tests on push CI -build=jepsen -test=core Run jepsen build and core tests on push CI -build=debug -test=core Run debug build and core tests on push CI -build=debug -test=integration Run debug build and integration tests on push CI -build=release -test=core Run release build and core tests on push CI -build=release -test=e2e Run release build and e2e tests on push CI -build=release -test=stress Run release build and stress tests on push CI -build=release -test=benchmark Run release build and benchmark on push labels Jul 19, 2024
@imilinovic imilinovic mentioned this pull request Jul 23, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 23, 2024
@imilinovic @Ignition
Properly handle query module errors when shutting down (#2225)
599c684 
#2176 introduced modules throwing upon failing to unload which wasn't
properly handled when shutting down Memgraph which could cause a crash.
Also modules were getting closed twice due to also getting closed in
destructor.

---------

Co-authored-by: Gareth Lloyd <[email protected]>
@Josipmrden Josipmrden mentioned this pull request Nov 21, 2024
Closed
as51340 pushed a commit that referenced this pull request Oct 24, 2025
@imilinovic @Ignition
Query module improvements (#2176)
6972be4 
Performance enhancements:
- C++ API `MemoryDispatcherGuard` had an expensive registration and
fetch mechanism, replaced with thread_local state
- Argument construction made temporary buffers unnecessarily
- Hot-loading required module lookup on every call, we modified
implementation to keep module lifetime for the whole function usage

---------

Co-authored-by: Gareth Lloyd <[email protected]>
as51340 pushed a commit that referenced this pull request Oct 24, 2025
@imilinovic @Ignition
Properly handle query module errors when shutting down (#2225)
9ab6339 
#2176 introduced modules throwing upon failing to unload which wasn't
properly handled when shutting down Memgraph which could cause a crash.
Also modules were getting closed twice due to also getting closed in
destructor.

---------

Co-authored-by: Gareth Lloyd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ignition Ignition Ignition left review comments

+2 more reviewers

@MarkoBarisic MarkoBarisic MarkoBarisic approved these changes

@antoniofilipovic antoniofilipovic antoniofilipovic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Ignition Ignition

@imilinovic imilinovic

Labels

breaking bug bug Code improvements Docs needed Docs needed

Projects

None yet

Milestone

mg-v2.18.1

Development

Successfully merging this pull request may close these issues.

Query modules in C++ are being congested by the unique lock

4 participants

@imilinovic @Ignition @antoniofilipovic @MarkoBarisic