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

[CARBONDATA-2184]Improve memory reuse for heap memory in `HeapMemoryAllocator` #1982

Closed
wants to merge 4 commits into from

Conversation

@zzcclp
Copy link
Contributor

zzcclp commented Feb 17, 2018

The description in SPARK-21860:
In HeapMemoryAllocator, when allocating memory from pool, and the key of pool is memory size.
Actually some size of memory ,such as 1025bytes,1026bytes,......1032bytes, we can think they are the same,because we allocate memory in multiples of 8 bytes.
In this case, we can improve memory reuse.

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

  • Any interfaces changed? No

  • Any backward compatibility impacted? No

  • Document update required? No

  • Testing done
    Please provide details on
    - Whether new unit test cases have been added or why no new tests are required?
    - How it is tested? Please attach test report.
    - Is it a performance related change? Please attach the performance test report.
    - Any additional information to help reviewers in testing this change.

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 17, 2018

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3754/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 17, 2018

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2514/

@zzcclp zzcclp closed this Feb 20, 2018
@zzcclp zzcclp reopened this Feb 20, 2018
@zzcclp

This comment has been minimized.

Copy link
Contributor Author

zzcclp commented Feb 20, 2018

@chenliang613 @jackylk please review, thanks.

@@ -44,38 +44,49 @@ private boolean shouldPool(long size) {
}

@Override public MemoryBlock allocate(long size) throws OutOfMemoryError {
if (shouldPool(size)) {
int numWords = (int) ((size + 7) / 8);

This comment has been minimized.

Copy link
@jackylk

jackylk Feb 21, 2018

Contributor

I think it is better to add a CarbonProperty to let user to enable this feature or not

This comment has been minimized.

Copy link
@zzcclp

zzcclp Feb 21, 2018

Author Contributor

do you mean the reuse feature or adjust the memory size?

This comment has been minimized.

Copy link
@jackylk

jackylk Feb 22, 2018

Contributor

I mean the reuse feature

This comment has been minimized.

Copy link
@zzcclp

zzcclp Feb 27, 2018

Author Contributor

Done

zzcclp added 2 commits Feb 17, 2018
…llocator`

The description in [SPARK-21860|https://issues.apache.org/jira/browse/SPARK-21860]:
In `HeapMemoryAllocator`, when allocating memory from pool, and the key of pool is memory size.
Actually some size of memory ,such as 1025bytes,1026bytes,......1032bytes, we can think they are the same,because we allocate memory in multiples of 8 bytes.
In this case, we can improve memory reuse.
@zzcclp zzcclp force-pushed the zzcclp:CARBONDATA-2184 branch from d591188 to 4c1def7 Feb 27, 2018
@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3917/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Failed with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2673/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3918/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2674/

@zzcclp

This comment has been minimized.

Copy link
Contributor Author

zzcclp commented Feb 27, 2018

@jackylk please help to review, thanks.

@ravipesala

This comment has been minimized.

Copy link
Contributor

ravipesala commented Feb 27, 2018

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3692/

bufferPoolsBySize = new HashMap<>();

private static final int POOLING_THRESHOLD_BYTES = 1024 * 1024;
private int poolingThresholdBytes = 1024 * 1024;

This comment has been minimized.

Copy link
@jackylk

jackylk Feb 27, 2018

Contributor

This assignment is not required

Assert.assertEquals(onheap4.size(), 1024 * 1024 + 7);
Assert.assertEquals(obj3, onheap4.getBaseObject());
}
}

This comment has been minimized.

Copy link
@jackylk

jackylk Feb 27, 2018

Contributor

please add one testcase to test if not pooling (set threshold to -1)

@ravipesala

This comment has been minimized.

Copy link
Contributor

ravipesala commented Feb 27, 2018

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3693/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Success with Spark 2.2.1, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/2686/

@CarbonDataQA

This comment has been minimized.

Copy link

CarbonDataQA commented Feb 27, 2018

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/3931/

@zzcclp

This comment has been minimized.

Copy link
Contributor Author

zzcclp commented Feb 27, 2018

retest sdv please.

@ravipesala

This comment has been minimized.

Copy link
Contributor

ravipesala commented Feb 27, 2018

SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/3704/

@jackylk

This comment has been minimized.

Copy link
Contributor

jackylk commented Feb 27, 2018

LGTM

@asfgit asfgit closed this in d0858b7 Feb 27, 2018
@zzcclp zzcclp deleted the zzcclp:CARBONDATA-2184 branch Feb 28, 2018
zzcclp added a commit to zzcclp/carbondata that referenced this pull request Mar 1, 2018
…llocator`

The description in [SPARK-21860|https://issues.apache.org/jira/browse/SPARK-21860]:
In `HeapMemoryAllocator`, when allocating memory from pool, and the key of pool is memory size.
Actually some size of memory ,such as 1025bytes,1026bytes,......1032bytes, we can think they are the same,because we allocate memory in multiples of 8 bytes.
In this case, we can improve memory reuse.

This closes apache#1982

(cherry picked from commit d0858b7)
asfgit pushed a commit that referenced this pull request Mar 3, 2018
…llocator`

The description in [SPARK-21860|https://issues.apache.org/jira/browse/SPARK-21860]:
In `HeapMemoryAllocator`, when allocating memory from pool, and the key of pool is memory size.
Actually some size of memory ,such as 1025bytes,1026bytes,......1032bytes, we can think they are the same,because we allocate memory in multiples of 8 bytes.
In this case, we can improve memory reuse.

This closes #1982
anubhav100 added a commit to anubhav100/incubator-carbondata that referenced this pull request Jun 22, 2018
…llocator`

The description in [SPARK-21860|https://issues.apache.org/jira/browse/SPARK-21860]:
In `HeapMemoryAllocator`, when allocating memory from pool, and the key of pool is memory size.
Actually some size of memory ,such as 1025bytes,1026bytes,......1032bytes, we can think they are the same,because we allocate memory in multiples of 8 bytes.
In this case, we can improve memory reuse.

This closes apache#1982
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.