----------------------------------------------------------- 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 > >
