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

[hail] Write memory management layer in scala #7033

Merged
merged 5 commits into from Sep 12, 2019

Conversation

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented Sep 9, 2019

No description provided.

@tpoterba
Copy link
Collaborator Author

@tpoterba tpoterba commented Sep 9, 2019

Still waiting on getting benchmarks working.

@cseed
Copy link
Collaborator

@cseed cseed commented Sep 10, 2019

Can we remove the corresponding Region C++ code, too?

@tpoterba
Copy link
Collaborator Author

@tpoterba tpoterba commented Sep 10, 2019

yes, will make that change afterwards.

@@ -397,6 +397,64 @@ class UnsafeSuite extends HailSuite {
p.check()
}

@Test def testRegionAllocationSimple() {
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

can you make sure that all of the (non-benchmark) tests in Region_test.cpp are accounted for here?

(also, maybe we should split these out into a separate RegionMemorySuite at some point)

Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

It might also be nice to add some checks on the total amount of memory allocated, since you're tracking that now.

}

def isValid: Boolean = _isValid
final class Region(var blockSize: Region.Size, var pool: RegionPool, var memory: RegionMemory = null) extends AutoCloseable {
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

is this constructor intended to be used elsewhere? Might be worth making it private.


def getReferenceCount: Long = referenceCount

def clear(): Unit = {
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

this should free large chunks.

Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

it feels like this and freeMemory want to do essentially the same thing, except that freeMemory also wants to release currentBlock and uninitialize the RegionMemory object by setting blockSize = -1. Might be worth breaking that part out into a separate function to make sure you're doing the same thing between the two calls.

Copy link
Collaborator Author

@tpoterba tpoterba Sep 11, 2019

Choose a reason for hiding this comment

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

good idea.


private def isFreed: Boolean = blockSize == -1

def freeMemory(): Unit = {
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

it feels kind of weird to me that this is public, since it feels like it's mostly an internal function. Actually, I think RegionMemory itself could probably be package-private, which would let Region/RegionPool access all of its stuff without exposing the memory management elsewhere.

Copy link
Collaborator Author

@tpoterba tpoterba Sep 10, 2019

Choose a reason for hiding this comment

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

yeah, that's what I want.

o
}

def allocate(n: Long): Long = {
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

I kind of feel like we could remove this signature of allocate all the way up. (its only use is in appendByte which is only used in some basic region tests.) Reduces the duplicated logic in this PR, but you don't have to do it here.


assert(pool.numRegions() == 1)
assert(pool.numFreeRegions() == 0)
assert(pool.numFreeBlocks() == 1)
Copy link
Contributor

@catoverdrive catoverdrive Sep 10, 2019

Choose a reason for hiding this comment

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

I'm not sure why we'd expect to see a free block appearing here---if we're allocating size-1 bytes into an empty region, we should see it allocating into an existing block in the first case, and grabbing a large chunk in the second. When it gets cleared, we should keep the current block and there shouldn't be any used blocks to return to the pool; the large chunk should get freed independently.

@danking danking merged commit 2056e54 into hail-is:master Sep 12, 2019
1 check passed
@tpoterba tpoterba mentioned this pull request Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants