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

Moving the majority of the memory manager interface to the slice #160

Merged
merged 5 commits into from
May 20, 2021

Conversation

sanastas
Copy link
Contributor

Moving the majority of the memory manager interface to the slice. And some other adaptations.

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Copy link
Contributor

@liran-funaro liran-funaro left a comment

Choose a reason for hiding this comment

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

This is an initial review.
I have some thoughts on how to disentangle ReferenceCodec from Slice that I can share with you online.

core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/Slice.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/Slice.java Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/Slice.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
public abstract AbstractSlice getDuplicatedSlice();

// Reset all common not final fields to invalid state
public abstract void invalidate();
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using a default implementation here that will initialize the common values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So how the specific fields will be invalidated?

core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
* Sets everything related to allocation of an off-heap cut: a portion of a bigger block.
* Turns empty slice to an associated slice upon allocation.
*/
void associateBlockAllocation(int blockID, int offset, int length, long memAddress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why isn't it a part of Slice's interface?
  • Could we associate a Slice to something other than a block allocation? If no, then this method can be renamed to associate().

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 not part of the Slice interface, because it is internal issue of the Memory Manager/Memory Allocator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are also other "associations" like version which is not part of the block allocation. So leaving the name as is meanwhile.

core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
Comment on lines 162 to 180
/**
* The slices are ordered by their length, then by their block id, then by their offset.
* Slices with the same length, block id and offset are considered identical.
*
* Used for comparision of the slices in the freeList of memory manager,
* the slices are deleted but not yet re-allocated.
*/
@Override
public int compareTo(Slice o) {
int cmp = Integer.compare(this.length, o.length);
if (cmp != 0) {
return cmp;
}
cmp = Integer.compare(this.blockID, o.blockID);
if (cmp != 0) {
return cmp;
}
return Integer.compare(this.offset, o.offset);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. I just understand the difference between Slice and AbstractSlice. See comment regarding this class name.

Copy link
Contributor

@liran-funaro liran-funaro left a comment

Choose a reason for hiding this comment

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

Looks good. See my comments below.

core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/AbstractSlice.java Outdated Show resolved Hide resolved
core/src/main/java/com/yahoo/oak/Block.java Outdated Show resolved Hide resolved
@sanastas sanastas merged commit 73b7881 into yahoo:master May 20, 2021
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