-
Notifications
You must be signed in to change notification settings - Fork 214
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
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed extra space
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.