Skip to content

Add new MemoryStatus interface to Theta. #589

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

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Conversation

leerho
Copy link
Contributor

@leerho leerho commented Aug 18, 2024

While going through the Java library, there are three public methods that had to do with the status of the internal Memory object used for a backing store: hasMemory(), isDirect(), and isSameResource(). These methods are only relevant to the sketch classes that need to access and retain the sketch as a Memory object, on or off-heap. But these three methods were inconsistently applied even to the classes that could use them. Some classes had only one or two of these methods implemented instead of all three.

Also because these methods in the root classes were implemented as abstract, it required all child dependents that did not need to access and retain the sketch as a Memory object, to implement these three methods as dummy methods that always returned false. This really clutters up the APIs of these classes and is "crazy-making" when trying to debug memory-related issues.

I thought it would be a good idea to clean this up. I have implemented a very simple interface, MemoryStatus, in o.a.ds.common with these three methods implemented as default methods that always return false. This way the only classes where these methods are relevant need to override these methods. And all the other classes don't need to implement dummy methods at all.

These methods are widely used across the library, but for this PR I have only implemented the necessary changes to the Theta tree to keep the PR smaller. Nonetheless there are 14 classes that were impacted, (an additional two classes had only spelling corrections). A summary of the changes for each file is included with the Files Changed View.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the new MemoryStatus interface that can be used eventually for the whole library. So far, this has only been applied to the Theta tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory not used as a backing store

  • Removed import to Memory
  • Removed isSameResource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isSameResource(Memory that) was missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a Base Interface

  • added extends MemoryStatus & import - default is OK here
  • removed hasMemory(), isDirect(), did not have isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory used as backing store

  • Updated hasMemory(), isDirect(), isSameResource() to check if mem != null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory used as backing store

  • Updated hasMemory(), isDirect(), isSameResource() to check if mem != null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory not used as a backing store

  • Removed hasMemory(), isDirect(), (did not have isSameResource())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory not used as a backing store

  • Removed hasMemory(), isDirect(), did not have isSameResource()
  • Note: need to remove getMemory()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory not used as a backing store

  • Removed hasMemory(), isDirect(), did not have isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory used as backing store

  • Added hasMemory(), isDirect(), updated isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base abstract class

  • Added implements MemoryStatus and import MemoryStatus
  • Removed abstract isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory not used as a backing store

  • Removed hasMemory(), isDirect(), did not have isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base abstract class

  • Added implements MemoryStatus and import MemoryStatus
  • Removed hasMemory(), isDirect(), isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Memory used as backing store via gadget

  • Added hasMemory(), isDirect(), updated isSameResource()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed extra space

@leerho leerho marked this pull request as draft August 18, 2024 22:59
@leerho leerho marked this pull request as ready for review August 19, 2024 23:44
@leerho leerho merged commit 3ee1750 into master Aug 20, 2024
4 checks passed
@leerho leerho deleted the add_MemoryStatus_to_theta branch August 20, 2024 15:56
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.

2 participants