You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2016/05/16 23:19:52 UTC
Review Request 47429: change defragment to not create fragments > 2G
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/
-----------------------------------------------------------
Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
Bugs: GEODE-1292
https://issues.apache.org/jira/browse/GEODE-1292
Repository: geode
Description
-------
change defragment to not create fragments > 2G
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
Diff: https://reviews.apache.org/r/47429/diff/
Testing
-------
precheckin
I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
Thanks,
Darrel Schneider
Re: Review Request 47429: change defragment to not create fragments >
2G
Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/#review133765
-----------------------------------------------------------
Ship it!
I agree with the original reasoning for naming method isSmallEnough. The test is for whether or not the combined size of adjacent free fragments is too big or too small to still be a valid fragment size; so if it 'isSmallEnough' then combine them.
- Ken Howe
On May 17, 2016, 11:13 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47429/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 11:13 p.m.)
>
>
> Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
>
>
> Bugs: GEODE-1292
> https://issues.apache.org/jira/browse/GEODE-1292
>
>
> Repository: geode
>
>
> Description
> -------
>
> change defragment to not create fragments > 2G
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectAddressStack.java b69d3a64cb09da953d196e112a6c635f80df0abb
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/SlabImpl.java 1c88bde9379c5408e15c4764da10db9ddb08a352
> geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/FreeListManagerTest.java 4cf1df00bb63154da03184c77f7656dbd892bbbb
>
> Diff: https://reviews.apache.org/r/47429/diff/
>
>
> Testing
> -------
>
> precheckin
>
> I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
>
>
> Thanks,
>
> Darrel Schneider
>
>
Re: Review Request 47429: change defragment to not create fragments >
2G
Posted by Darrel Schneider <ds...@pivotal.io>.
> On May 17, 2016, 5:56 p.m., Sai Boorlagadda wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java, line 338
> > <https://reviews.apache.org/r/47429/diff/1-2/?file=1384862#file1384862line338>
> >
> > Should it be called as isBigEnough or isLargeEnough?
I don't think so. It returns true if the size is small enough to be used as the size of a Fragment.
If the size is too big then it will return false. So the statement "if isSmallEnough(size)" would be read as if "size" is small enough then ...
- Darrel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/#review133673
-----------------------------------------------------------
On May 17, 2016, 4:13 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47429/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 4:13 p.m.)
>
>
> Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
>
>
> Bugs: GEODE-1292
> https://issues.apache.org/jira/browse/GEODE-1292
>
>
> Repository: geode
>
>
> Description
> -------
>
> change defragment to not create fragments > 2G
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectAddressStack.java b69d3a64cb09da953d196e112a6c635f80df0abb
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/SlabImpl.java 1c88bde9379c5408e15c4764da10db9ddb08a352
> geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/FreeListManagerTest.java 4cf1df00bb63154da03184c77f7656dbd892bbbb
>
> Diff: https://reviews.apache.org/r/47429/diff/
>
>
> Testing
> -------
>
> precheckin
>
> I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
>
>
> Thanks,
>
> Darrel Schneider
>
>
Re: Review Request 47429: change defragment to not create fragments >
2G
Posted by Sai Boorlagadda <sb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/#review133673
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java (line 338)
<https://reviews.apache.org/r/47429/#comment198247>
Should it be called as isBigEnough or isLargeEnough?
- Sai Boorlagadda
On May 17, 2016, 11:13 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47429/
> -----------------------------------------------------------
>
> (Updated May 17, 2016, 11:13 p.m.)
>
>
> Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
>
>
> Bugs: GEODE-1292
> https://issues.apache.org/jira/browse/GEODE-1292
>
>
> Repository: geode
>
>
> Description
> -------
>
> change defragment to not create fragments > 2G
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectAddressStack.java b69d3a64cb09da953d196e112a6c635f80df0abb
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/SlabImpl.java 1c88bde9379c5408e15c4764da10db9ddb08a352
> geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/FreeListManagerTest.java 4cf1df00bb63154da03184c77f7656dbd892bbbb
>
> Diff: https://reviews.apache.org/r/47429/diff/
>
>
> Testing
> -------
>
> precheckin
>
> I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
>
>
> Thanks,
>
> Darrel Schneider
>
>
Re: Review Request 47429: change defragment to not create fragments >
2G
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/
-----------------------------------------------------------
(Updated May 17, 2016, 4:13 p.m.)
Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
Changes
-------
Add unit test coverage for chunks that can not be combined because they are too big.
Fixed a TODO in the unit test.
Added unit tests for isAdjacent and isSmallEnough.
Added unit test for SlabImpl.toString.
Code coverage of FreeListManager has improved with these changes.
Bugs: GEODE-1292
https://issues.apache.org/jira/browse/GEODE-1292
Repository: geode
Description
-------
change defragment to not create fragments > 2G
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/OffHeapStoredObjectAddressStack.java b69d3a64cb09da953d196e112a6c635f80df0abb
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/SlabImpl.java 1c88bde9379c5408e15c4764da10db9ddb08a352
geode-core/src/test/java/com/gemstone/gemfire/internal/offheap/FreeListManagerTest.java 4cf1df00bb63154da03184c77f7656dbd892bbbb
Diff: https://reviews.apache.org/r/47429/diff/
Testing
-------
precheckin
I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
Thanks,
Darrel Schneider
Re: Review Request 47429: change defragment to not create fragments >
2G
Posted by Sai Boorlagadda <sb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47429/#review133559
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java (line 310)
<https://reviews.apache.org/r/47429/#comment198056>
Pass lowSize as an argument to be able to unit test combineIfAdjacent, so that it can get coverage.
Also mocking combineIfAdjacent to return false we can test the code path with out allocating adjacent memories bigger than 2G
- Sai Boorlagadda
On May 16, 2016, 11:19 p.m., Darrel Schneider wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47429/
> -----------------------------------------------------------
>
> (Updated May 16, 2016, 11:19 p.m.)
>
>
> Review request for geode, Eric Shu, Scott Jewell, Ken Howe, and Sai Boorlagadda.
>
>
> Bugs: GEODE-1292
> https://issues.apache.org/jira/browse/GEODE-1292
>
>
> Repository: geode
>
>
> Description
> -------
>
> change defragment to not create fragments > 2G
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/offheap/FreeListManager.java c5e17e860e8e3300aac490621ae5d5b50f3a0e38
>
> Diff: https://reviews.apache.org/r/47429/diff/
>
>
> Testing
> -------
>
> precheckin
>
> I looked over the code to see if I could create a unit test that does not need to allocate 2G chunks of off-heap memory and it was not possible without major refactoring. The problem is that we modify size fields stored at an address during defragmentation and currently that requires that the address be a valid off-heap memory address.
>
>
> Thanks,
>
> Darrel Schneider
>
>