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

Refactor arena & pool guards and logs #700

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

gavv
Copy link
Member

@gavv gavv commented Feb 13, 2024

  • Instead of more generic "flags" enum, use more specific "guards", so that it's easier to understand the purpose

  • Use separate flag for each type of guard

  • Extract method that increments failure counter and checks if panics are enabled for a guard

  • Use same logging format as in MemoryLimiter

Copy link

🤖 Upon creation, pull request description does not have a link to an issue. If there is a related issue, please add it to the description using any of the supported formats.

- Instead of more generic "flags" enum, use more specific "guards",
  so that it's easier to understand the purpose

- Use separate flag for each type of guard

- Extract method that increments failure counter and checks
  if panics are enabled for a guard

- Use same logging format as in MemoryLimiter
@gavv gavv added the ready for review Pull request can be reviewed label Feb 13, 2024
//! Default memory pool guards.
enum {
SlabPool_DefaultGuards =
(SlabPool_LeakGuard | SlabPool_OverflowGuard | SlabPool_OwnershipGuard)
Copy link
Contributor

Choose a reason for hiding this comment

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

With HeapArena, we don't want leak guard being the default, but, over here, we're ok with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes,

We would enable it for heap arena too, but when roc is used from a C library, some allocation & deallocation requests come from the user. We don't want to crash user's app if user decides not to free some resources upon exit, for example. So leak detection is disabled by default and is enabled explicitly in tools and tests.

All requests to pool, on the other hand, are controlled only by us, so we can place stricter defaults.

Copy link
Contributor

@nolan-veed nolan-veed left a comment

Choose a reason for hiding this comment

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

Nice one!

@gavv gavv merged commit bb44541 into roc-streaming:develop Apr 9, 2024
32 checks passed
@gavv gavv deleted the feature/guards branch April 9, 2024 17:14
@gavv
Copy link
Member Author

gavv commented Apr 9, 2024

Thanks for review!

@github-actions github-actions bot removed the ready for review Pull request can be reviewed label Apr 9, 2024
@gavv gavv added this to the next milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants