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

Add atomic memory block around unsafe code blocks #1589

Merged
merged 8 commits into from Dec 21, 2023

Conversation

antoniofilipovic
Copy link
Collaborator

@antoniofilipovic antoniofilipovic commented Dec 15, 2023

Fixes https://github.com/memgraph/memgraph/actions/runs/7100898041/job/19359933556

Slack thread: https://memgraph.slack.com/archives/CS5827FK3/p1702652195334839?thread_ts=1702627200.164709&cid=CS5827FK3

If idea seems good, I will apply this to rest of operations in vertex_accessor and edge_accessor which from memory allocation should be "atomic".

[master < Task] PR

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

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

  • Write a release note here, including added/changed clauses
  • Tag someone from docs team in the comments

Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

I don't understand why we need DoCheck?

src/utils/oom_blocker_block.hpp Outdated Show resolved Hide resolved
src/storage/v2/vertex_accessor.cpp Outdated Show resolved Hide resolved
src/utils/memory_tracker.hpp Show resolved Hide resolved
src/storage/v2/vertex_accessor.cpp Outdated Show resolved Hide resolved
@antoniofilipovic antoniofilipovic changed the title Fix flaky memory limit test Add atomic memory block around unsafe code blocks Dec 18, 2023
@antoniofilipovic antoniofilipovic marked this pull request as ready for review December 18, 2023 15:01
Copy link
Contributor

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

💪

Copy link
Contributor

@Ignition Ignition left a comment

Choose a reason for hiding this comment

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

Rather than block one kind of exception, why not make the code exception safe? e.g. if SetProperty has memory exception then need to unlink and delete delta inside a catch + rethrow

@antoniofilipovic
Copy link
Collaborator Author

antoniofilipovic commented Dec 19, 2023

Rather than block one kind of exception, why not make the code exception safe? e.g. if SetProperty has memory exception then need to unlink and delete delta inside a catch + rethrow

@Ignition I would say it would make code even more dependent on OOM Exception checks. If I understood correctly, in that case I would need to have Try Catch blocks inside the SetProperty function in VertexAccessor, which means that an exception would be thrown, we will have stack unwinding there and afterward we need to process exception and do additional steps to revert changes. This way it is in my opinion more performant and "cleaner" to have Atomic memory block, which doesn't allow to throw exception, and it is more reusable also. There is no exception thrown until needed.

Copy link
Contributor

@Ignition Ignition left a comment

Choose a reason for hiding this comment

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

For we need now, this works.
One small change requested.

@antoniofilipovic antoniofilipovic merged commit cd37de4 into master Dec 21, 2023
6 checks passed
@antoniofilipovic antoniofilipovic deleted the fix-flaky-memory-tracker branch December 21, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs - changelog only Docs - changelog only memory
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants