giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Avery Ching" <avery.ch...@gmail.com>
Subject Re: Review Request 12174: Add "one-to-all" message sending strategy
Date Mon, 08 Jul 2013 18:40:32 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12174/#review22838
-----------------------------------------------------------


This needs a bit of work, but the performance seems pretty good from your experiments.  Please
address the below questions/comments.  Don't forget to rebase =).


giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java
<https://reviews.apache.org/r/12174/#comment46546>

    Since you do not change values, it is safer to make this private and then have a getter
for the individual buffer sizes.  I.e. getInitialBufferSize(int workerIndex) or something
like that.



giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java
<https://reviews.apache.org/r/12174/#comment46544>

    Why did you change this.  It was private with a getter before?



giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java
<https://reviews.apache.org/r/12174/#comment46545>

    Want to make this private with a getter as well?



giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java
<https://reviews.apache.org/r/12174/#comment46554>

    Can we factor out the common code here?  The difference is only one line.



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java
<https://reviews.apache.org/r/12174/#comment46557>

    Flush the rest of the messages to the workers



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46550>

    extra space



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46563>

    Indent.



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46560>

    So don't worry weather =>
    Therefore, it doesn't matter if the result is null or not.



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46561>

    indent wrong



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46562>

    that belong



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46551>

    You must rethrow, say as an IllegalStateException.  This will let errors through.



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46564>

    space.



giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java
<https://reviews.apache.org/r/12174/#comment46567>

    This is wrong now I think.



giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java
<https://reviews.apache.org/r/12174/#comment46568>

    Nice job refactoring this out.



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46569>

    extra space.  Did you run with verify?  It should have caught this.



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46552>

    extra space



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46570>

    Also, please explain the logic for the 2x buffer size.



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46573>

    extra space!



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46572>

    Please break line after the .



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46574>

    Isn't this expensive to do the copy into a format that can be processed by the same code
as the original send messages?  Can't we avoid a buffer copy by directly deserializing and
adding to the message store directly?  Also, is this compatible with changing the message
store between supersteps?



giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
<https://reviews.apache.org/r/12174/#comment46571>

    Not necessary.  You are throwing in the next line.



giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayOneToAllMessages.java
<https://reviews.apache.org/r/12174/#comment46575>

    I don't think these are necessary right?  Your request has access to conf.



giraph-hcatalog/src/main/java/org/apache/giraph/io/hcatalog/GiraphHCatInputFormat.java
<https://reviews.apache.org/r/12174/#comment46553>

    This is not your change right?  From trunk?


- Avery Ching


On July 1, 2013, 12:16 a.m., Bingjing Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12174/
> -----------------------------------------------------------
> 
> (Updated July 1, 2013, 12:16 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Bugs: GIRAPH-701
>     https://issues.apache.org/jira/browse/GIRAPH-701
> 
> 
> Repository: giraph-git
> 
> 
> Description
> -------
> 
> Add "one-to-all" message sending strategy.
> Now when sendMessageToAllEdges is invoked, Giraph may apply "one-to-all" message sending
strategy.
> To enable it, use conf.enableOneToAllMsgSending() 
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java 92d0926 
>   giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java 40023c2 
>   giraph-core/src/main/java/org/apache/giraph/comm/SendMessageToAllCache.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/comm/ServerData.java affc260 
>   giraph-core/src/main/java/org/apache/giraph/comm/WorkerClientRequestProcessor.java
89fb3e4 
>   giraph-core/src/main/java/org/apache/giraph/comm/messages/OneMessagePerVertexStore.java
f18af5b 
>   giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java
d786db5 
>   giraph-core/src/main/java/org/apache/giraph/comm/requests/RequestType.java 4129fb8

>   giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java
PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 2d232a6 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java c65b5f0 
>   giraph-core/src/main/java/org/apache/giraph/graph/Computation.java 87d5879 
>   giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayOneToAllMessages.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdData.java 9b3f165

>   giraph-core/src/test/java/org/apache/giraph/comm/RequestTest.java c8c09df 
>   giraph-core/src/test/java/org/apache/giraph/utils/MockUtils.java bc5b5e2 
>   giraph-hcatalog/src/main/java/org/apache/giraph/io/hcatalog/GiraphHCatInputFormat.java
2e91cba 
> 
> Diff: https://reviews.apache.org/r/12174/diff/
> 
> 
> Testing
> -------
> 
> Did
> mvn clean verify
> mvn clean test
> 
> 
> Thanks,
> 
> Bingjing Zhang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message