[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore #222

hy00nc · 2019-06-18T07:25:55Z

JIRA: NEMO-350: Implement Off-heap SerializedMemoryStore
NEMO-384: Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore

Major changes:

When a block is emitted by an executor, we write it directly to off-heap memory using DirectByteBufferOutputStream and DirectByteBufferOutputStream.

Minor changes to note:

getData() and getBuffer should be distinguished when acquiring data in SerializedPartition

Other comments:

This implementation does not ensure performance gain since the overhead of allocateDirect (malloc) surpasses the garbage collection overhead. For this reason, memory management is being implemented.

…ream

…ffHeapTest

…into offHeapTest

…ffHeapTest

…into Off-Heap_PR

johnyangk

Thanks @hy00nc! I've left some comments. I also have two questions regarding the issue description.

First, I was not able to find in the code "If there is any chance of using getData() in SerializedPartition, we check the length of the SerializedData to ensure that this SerializedPartition was created from existing data". Can you point me to it?

Second, can you file a JIRA issue and add a TODO comment besides allocateDirect? That would make it clear that line of code causes overheads.

e.g.,
// TODO #999: ISSUE_TITLE

johnyangk · 2019-06-19T05:58:14Z

common/src/main/java/org/apache/nemo/common/DirectByteBufferInputStream.java

+   */
+  @Override
+  public int read() throws IOException {
+    return getBuffer().get() & 0xff;


Can you make 0xff a static variable with a descriptive name?
I am curious why 0xff is needed here. Is there perhaps a link to a document that I can refer to? It'd be great to mention that in the comment as well.

You can probably refer to this website: https://rules.sonarsource.com/java/RSPEC-3034 . As far as I know, this is a java-specific issue. Since the raw byte value in java represents signed value, it should be masked to get the proper value. I will mention this in the comment thanks :)

johnyangk · 2019-06-19T06:04:25Z

common/src/main/java/org/apache/nemo/common/DirectByteBufferInputStream.java

+ * list of {@link ByteBuffer}.
+ */
+public class DirectByteBufferInputStream extends InputStream {
+  private List<ByteBuffer> bufList;


Please add a comment something like:
// The contents of direct buffers may reside outside of the normal garbage-collected heap

I found the above in the ByteBuffer documentation. Is there a way to "guarantee" that the contents reside outside of the heap?

My understanding is that it can be done by using DirectByteBuffer, rather than the abstract class ByteBuffer. It'd be good to make this change, also since the name of this class is DirectByteBufferInputStream.
https://www.javacodegeeks.com/2013/08/which-memory-is-faster-heap-or-bytebuffer-or-direct.html

I think you're right, the name of the class implies that this is only for DirectByteBuffer. I guess it is better to change the name of the class to ByteBufferInputStream because DirectByteBuffer class is not public and it can only be constructed by calling allocateDirect() method in ByteBuffer. It is possible to check whether the ByteBuffer is direct or not, but I am afraid it has no much meaning in checking it when it is already written(in on-heap or off-heap) and we are intending to read it. Does it seem okay?

Agreed.
I am also fine with keeping the current class name DirectByteBufferInputStream, as we also have DirectByteBufferOutputStream.

common/src/main/java/org/apache/nemo/common/DirectByteBufferOutputStream.java

runtime/executor/src/main/java/org/apache/nemo/runtime/executor/data/block/FileBlock.java

...cutor/src/main/java/org/apache/nemo/runtime/executor/data/partition/SerializedPartition.java

common/src/main/java/org/apache/nemo/common/DirectByteBufferOutputStream.java

hy00nc · 2019-06-19T07:39:31Z

Thanks @hy00nc! I've left some comments. I also have two questions regarding the issue description.

First, I was not able to find in the code "If there is any chance of using getData() in SerializedPartition, we check the length of the SerializedData to ensure that this SerializedPartition was created from existing data". Can you point me to it?

Second, can you file a JIRA issue and add a TODO comment besides allocateDirect? That would make it clear that line of code causes overheads.

e.g.,
// TODO #999: ISSUE_TITLE

Thanks for the review @johnyangk !

For the first question, I didn't update the comment after addressing some fixes for the bug. Sorry for the confusion. Please check the updated comment :)

For the second one, maybe I can add to the existing JIRA about the overhead. I will also add the TODO in the code, thanks!

johnyangk

Thanks @hy00nc! The code looks good.
I've left minor questions. I'll merge as soon as you confirm.

johnyangk · 2019-06-25T05:25:57Z

common/src/main/java/org/apache/nemo/common/DirectByteBufferInputStream.java

+ * list of {@link ByteBuffer}.
+ */
+public class DirectByteBufferInputStream extends InputStream {
+  private List<ByteBuffer> bufList;


Agreed.
I am also fine with keeping the current class name DirectByteBufferInputStream, as we also have DirectByteBufferOutputStream.

common/src/main/java/org/apache/nemo/common/DirectByteBufferOutputStream.java

runtime/executor/src/main/java/org/apache/nemo/runtime/executor/data/block/FileBlock.java

...cutor/src/main/java/org/apache/nemo/runtime/executor/data/partition/SerializedPartition.java

hy00nc · 2019-06-25T06:15:39Z

Thanks @hy00nc! The code looks good.
I've left minor questions. I'll merge as soon as you confirm.

Thanks again! I made minor changes and it's ready to be merged :)

…ement DirectByteBufferInputStream for Off-heap SerializedMemoryStore (apache#222) JIRA: [NEMO-350: Implement Off-heap SerializedMemoryStore](https://issues.apache.org/jira/projects/NEMO/issues/NEMO-#350) [NEMO-384: Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore](https://issues.apache.org/jira/projects/NEMO/issues/NEMO-#384) **Major changes:** - When a block is emitted by an executor, we write it directly to off-heap memory using `DirectByteBufferOutputStream` and `DirectByteBufferOutputStream`. **Minor changes to note:** - `getData()` and `getBuffer` should be distinguished when acquiring data in `SerializedPartition` **Other comments:** - This implementation does not ensure performance gain since the overhead of `allocateDirect` (malloc) surpasses the garbage collection overhead. For this reason, memory management is being implemented.

hy00nc and others added 30 commits April 17, 2019 13:37

substitution: DirectByteArrayOutputstream to DirectByteBufferOutputSt…

26f364f

…ream

TPC-H application codes

4ded359

TPC-H application codes

8c557d9

TPC-H application codes

3ec9c8e

beam api changed

1dc1dd8

beam tests

666cd01

logging

05e7033

page size experiment 32KB

ceba288

initial implementation

0ab5c93

getBufferList fix

940bef7

revert and disable clearing list for now

9789d7e

tests

34848af

For writeToFile experiment...

d01c0f0

no stream

87409c7

logging

6a296a6

use SerializedMemoryStore

4d349df

logging

5766022

logging

e6b271e

new edge annotation

767c182

Return duplicated buffer when calling getBufferList()

e40064b

Small fix

c75ec2c

tests for getData()

a1bc8d6

Fix toByteArray()

64b9df2

Merge branch 'offHeapTest' of github.com:hy00nc/incubator-nemo into o…

47c9832

…ffHeapTest

revert customized DefaultDataStorePass

8f77496

Merge branch 'offHeapTest' of https://github.com/hy00nc/incubator-nemo …

19bad77

…into offHeapTest

Implement zero-copy file block write

e1be1c4

Merge branch 'offHeapTest' of github.com:hy00nc/incubator-nemo into o…

597c2f1

…ffHeapTest

Fix type conversion error

5339c3a

default + serializedmemorystore annotatation

b1b7eb0

hy00nc added 17 commits May 13, 2019 15:48

new Constructor for SerializedPartition

7253e6f

offheap for converting non-serialized data to serialized data

93ecf76

initial edits

edd1975

erase useless codes

53faa33

ByteOutputContext

58925d3

comments and erases

d56494e

test

39412ad

file channel use refactoring

fa49ca3

file channel fix

fbf97e2

checkstyle

48c6b98

getData() exception

6f4dae5

checkstyle

e5ef592

checkstyle

d0e5478

Merge branch 'master' into Off-Heap_PR

0cb1f1e

getBuffer() fixed to return the duplicated list of ByteBuffers.

17cd1d7

fixed to pass BlockStoreTest

7b4a6bc

Merge branch 'Off-Heap_PR' of https://github.com/hy00nc/incubator-nemo …

617dfd3

…into Off-Heap_PR

johnyangk suggested changes Jun 19, 2019

View reviewed changes

hy00nc added 3 commits June 21, 2019 14:46

refactoring

1b512c4

implements autocloseable

449649d

refactoring

38578df

johnyangk reviewed Jun 25, 2019

View reviewed changes

hy00nc added 2 commits June 25, 2019 14:50

rename class ByteBufferInputStream

c55d54d

minor comment editing

bf689d2

johnyangk approved these changes Jun 25, 2019

View reviewed changes

johnyangk merged commit ea448db into apache:master Jun 25, 2019

hy00nc mentioned this pull request Jul 1, 2019

[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Impl… hy00nc/incubator-nemo#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore #222

[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore #222

hy00nc commented Jun 18, 2019 •

edited

Loading

johnyangk left a comment

johnyangk Jun 19, 2019

hy00nc Jun 21, 2019

johnyangk Jun 19, 2019

hy00nc Jun 21, 2019 •

edited

Loading

johnyangk Jun 25, 2019

hy00nc commented Jun 19, 2019 •

edited

Loading

johnyangk left a comment

johnyangk Jun 25, 2019

hy00nc commented Jun 25, 2019

[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore #222

[NEMO-350] Implement Off-heap SerializedMemoryStore & [NEMO-384] Implement DirectByteBufferInputStream for Off-heap SerializedMemoryStore #222

Conversation

hy00nc commented Jun 18, 2019 • edited Loading

johnyangk left a comment

Choose a reason for hiding this comment

johnyangk Jun 19, 2019

Choose a reason for hiding this comment

hy00nc Jun 21, 2019

Choose a reason for hiding this comment

johnyangk Jun 19, 2019

Choose a reason for hiding this comment

hy00nc Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

johnyangk Jun 25, 2019

Choose a reason for hiding this comment

hy00nc commented Jun 19, 2019 • edited Loading

johnyangk left a comment

Choose a reason for hiding this comment

johnyangk Jun 25, 2019

Choose a reason for hiding this comment

hy00nc commented Jun 25, 2019

hy00nc commented Jun 18, 2019 •

edited

Loading

hy00nc Jun 21, 2019 •

edited

Loading

hy00nc commented Jun 19, 2019 •

edited

Loading