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

Enhancement: Reducing memory usage by removing the vtable in managed. #874

Closed
wants to merge 8 commits into from

Conversation

RealTimeChris
Copy link
Member

Have applied the CRTP to keep the interfaces identical, as well as added snowflakes to the necessary classes, and also added a managed_base class to allow for storing them all in the same deletion_queue.

Code change checklist

  • I have ensured that all methods and functions are fully documented using doxygen style comments
  • My code follows the coding style guide.
  • I tested that my change works before raising the PR.
  • I have ensured that I did not break any existing API calls.
  • I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

Realized that I had done this wrong in the previous implementation. Also edited the name of a variable. Also, just edited the loads to use aligned loads instead of unaligned.
…well as encapsulating some of their functions.

Can't believe I didn't notice this before.
Have applied the CRTP to keep the interfaces identical, as well as added snowflakes to the necessary classes.
@netlify
Copy link

netlify bot commented Sep 21, 2023

Deploy Preview for dpp-dev ready!

Name Link
🔨 Latest commit 7004256
🔍 Latest deploy log https://app.netlify.com/sites/dpp-dev/deploys/650c9b95146b030008428cc4
😎 Deploy Preview https://deploy-preview-874--dpp-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

public:
//** Unique identifier */
snowflake id;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like this, it's basically added tons of duplicate lines to every object to save 8 bytes of ram per instance, I think readability is more important in this case

@Mishura4
Copy link
Member

The id could be part of managed, that would help with the code duplication

@RealTimeChris
Copy link
Member Author

The id could be part of managed, that would help with the code duplication

It was deliberately removed from managed in order to eliminate the vtable.

@Mishura4
Copy link
Member

The id could be part of managed, that would help with the code duplication

It was deliberately removed from managed in order to eliminate the vtable.

Having just the snowflake in there isn't going to produce a vtable though.

@RealTimeChris
Copy link
Member Author

RealTimeChris commented Sep 21, 2023

The id could be part of managed, that would help with the code duplication

It was deliberately removed from managed in order to eliminate the vtable.

Having just the snowflake in there isn't going to produce a vtable though.

No but having the virtual destructor that is then required to properly destroy the snowflake will.

@Mishura4
Copy link
Member

Virtual destructor is needed if you want to destroy a variable through a pointer to its base class, so the derived object is properly destroyed. Whether the snowflake is in the base or not doesn't change that

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.

3 participants