> On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > Avery, this is awesome! It will improve the performance of vertex input, > > and remove the need for preprocessing data in some applications.
Thanks for the fast review. > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java, > > line 104 > > <https://reviews.apache.org/r/14527/diff/2/?file=362354#file362354line104> > > > > Why don't we use the return value of putOrCombine to decide whether to > > release or not? Thanks for noticing! I forget that is why I even added a return value to putOrCombine...=) > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java, > > line 142 > > <https://reviews.apache.org/r/14527/diff/1/?file=362255#file362255line142> > > > > I think this is more inefficient than it needs to be. How about we call > > putIfAbsent, and then only if that doesn't succeed we synchronize? And when > > we synchronize, maybe we can just synchronize on the vertex id object from > > the map instead of the whole partition? Nice idea. I changed it to use putIfAbsent(). But synching on the id is hard since we can't get the object and value is not easy, since we end up replacing it. Given that we are adding the partition, locking at the partition level might be more performant. > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java, > > line 163 > > <https://reviews.apache.org/r/14527/diff/1/?file=362255#file362255line163> > > > > Same question as above This is a bit different than the above case since it's already in the form of a vertex. While this is a tradeoff, the approach you suggest is optimistic, and the chances of combining as less generally, so I changed it as you suggested. Good eye! > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/partition/DiskBackedPartitionStore.java, > > line 225 > > <https://reviews.apache.org/r/14527/diff/1/?file=362256#file362256line225> > > > > Why this needed to be modified? This is useful for SendWorkerVerticesRequest since it might have to create partitions. > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/comm/SendVertexIdDataCache.java, > > lines 81-82 > > <https://reviews.apache.org/r/14527/diff/1/?file=362225#file362225line81> > > > > It's not clean that all implementations have to care about updating the > > size whenever they modify data in the super class. Although I'm not sure > > how to do it better, so if you don't either I'm fine with it staying this > > way. I am not sure either since the underlying data can be anything. We could add an API where the underlying data keeps track of the size I suppose. Maybe we try this later. > On Oct. 8, 2013, 6:56 p.m., Maja Kabiljo wrote: > > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerVerticesRequest.java, > > line 123 > > <https://reviews.apache.org/r/14527/diff/1/?file=362239#file362239line123> > > > > Add comment describing where 13 comes from Changed to 8 bytes and explained why. - Avery ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14527/#review26775 ----------------------------------------------------------- On Oct. 9, 2013, 5:40 a.m., Avery Ching wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14527/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2013, 5:40 a.m.) > > > Review request for giraph. > > > Bugs: GIRAPH-775 > https://issues.apache.org/jira/browse/GIRAPH-775 > > > Repository: giraph-git > > > Description > ------- > > * Implements vertex value combining at load time. > * Vertices are sent to destination workers via a immediately serialized > vertex cache instead of a cached partition > * Fixed a bug where a vertex input format's edges would be overwritten by an > edge input format's edges > * Changed the name of combiner to message combiner and vertex value combiner > to be more explicit - hence the large number of changes. > * Added new IntIntNullTextVertexInputFormat that takes in id, value, edges > > Added 2 new unittests: > * Testing vertex value combiner > * Testing mixed vertex and edge input formats with overlapping edges > > > Diffs > ----- > > CHANGELOG f960d0904f1bfaea8b2b7cac60b3e37e7bca7ba2 > > giraph-core/src/main/java/org/apache/giraph/benchmark/PageRankBenchmark.java > acc1c461abc04d9a1e7d1128e6fe3f5fb06ca4e9 > > giraph-core/src/main/java/org/apache/giraph/benchmark/ShortestPathsBenchmark.java > 0dd4529332f0fe7223be194c64337f4aa95a75b6 > > giraph-core/src/main/java/org/apache/giraph/benchmark/WeightedPageRankBenchmark.java > 2077674fd104c7275e6d086b26faa7039b7091e1 > giraph-core/src/main/java/org/apache/giraph/combiner/Combiner.java > 7830ffff079c945eb8b0258a733ff72dc426b797 > giraph-core/src/main/java/org/apache/giraph/combiner/DoubleSumCombiner.java > 8da4ba77b6f68d8f901b9aa4f2ecc439eec5434d > giraph-core/src/main/java/org/apache/giraph/combiner/FloatSumCombiner.java > d89879150efa7ee0df9845f26dc9390a9b632171 > > giraph-core/src/main/java/org/apache/giraph/combiner/MinimumDoubleCombiner.java > 0a85d64c388582e1f7d25c57da2772ada51f9ea5 > > giraph-core/src/main/java/org/apache/giraph/combiner/MinimumIntCombiner.java > fcef58eaec13c22ffa01c34d0a3ec29ab0fee53d > giraph-core/src/main/java/org/apache/giraph/combiner/SimpleSumCombiner.java > 2a11d2f06be9365ec8ef6dae0759a9e47a4f08c0 > giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java > 30c07ee957b66b9c11495cabf3719f94e445acf1 > giraph-core/src/main/java/org/apache/giraph/comm/SendEdgeCache.java > 5513da2a6a4bed63ae0915fb257e830b6c077fa5 > giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java > 8df0dda628e1753896eaa0ed96a5383318056612 > giraph-core/src/main/java/org/apache/giraph/comm/SendPartitionCache.java > 524c9f1cf6a605d9e06b216b980b8e21712d7af5 > giraph-core/src/main/java/org/apache/giraph/comm/SendVertexIdDataCache.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/comm/WorkerClientRequestProcessor.java > 9bdf9cae20b47b5063b343e1a9d42babfd37df96 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/InMemoryMessageStoreFactory.java > 0cdfb733014496b84f26fceb35cb9f509ac1eac7 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/MessageStoreFactory.java > 254afd404716ecb4793a2f22dc336ddf53e20242 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/OneMessagePerVertexStore.java > 4f150daeaf639a6f4de6ba6b45d47f514e7f38cc > > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/IntByteArrayMessageStore.java > cdab2e0c647f7d4a578db016ea1c27d362b6c8a7 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/IntFloatMessageStore.java > a193fb97ce6e0a86fbfd61dee6dd3d8249c2ad8b > > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/LongByteArrayMessageStore.java > 3272ced26b66da56da7413430dedadaeadbafafd > > giraph-core/src/main/java/org/apache/giraph/comm/messages/primitives/LongDoubleMessageStore.java > 96ed5b4981c670f0534d22bce406b4d39796d4e9 > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClient.java > 28f365693ce3e8701fa3cb72ea8683733ea3691e > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java > 34a3d1ff86a4f73ecefad4891a5d8d04368e653b > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerServer.java > 3473de1c57c05333c5731775ef85a19c95717a28 > > giraph-core/src/main/java/org/apache/giraph/comm/netty/handler/RequestEncoder.java > 83b408e3102696a859acb5c0286b541f44bacddf > giraph-core/src/main/java/org/apache/giraph/comm/requests/RequestType.java > a1dcece230d63f7b3bed583ea3bc437b481eab03 > > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendVertexRequest.java > e0cb916668a477d2bebd4df22aa697017b1a2c78 > > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerVerticesRequest.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java > f97446fbc3c46365338fb1ef33e203fa2266eda6 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java > 15ff861ca062b8981b6dd2a385b4e64f7653b968 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java > 4dadd29d8f82ccc7b918d9e190c7f439c4da7134 > > giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java > 435dfa5679e6c4738d649df0cd02f1387950eab8 > giraph-core/src/main/java/org/apache/giraph/edge/EdgeStore.java > 23df689121944d834803ffcba47cb5f61cb74451 > giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java > 77d9f5ebc5a283794c1e26b0d5f0a723e018f766 > > giraph-core/src/main/java/org/apache/giraph/graph/DefaultVertexValueCombiner.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/graph/VertexValueCombiner.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/IntIntNullTextVertexInputFormat.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/io/formats/TextVertexValueInputFormat.java > 6a795a8fe640321b34ef689315436fa6c73132bd > > giraph-core/src/main/java/org/apache/giraph/job/GiraphConfigurationValidator.java > 5b870c54e92a7ece943bef04e3264e5703cb815a > giraph-core/src/main/java/org/apache/giraph/jython/JythonJob.java > 6b2eedf8bf82bf13e10eb20e2d98b8a960348844 > giraph-core/src/main/java/org/apache/giraph/master/MasterCompute.java > cf7356cf9f98ef120b7d6bd14af421e9bceeacf2 > giraph-core/src/main/java/org/apache/giraph/master/SuperstepClasses.java > 7a7df052e8b7b937b14d032875f5301a02ea9a8f > giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java > f2b855249c6a126d2e042048489e71baa6d65ea4 > > giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java > 6eaa6d76efac80c53251ad1037f1c44ed6c4d933 > > giraph-core/src/main/java/org/apache/giraph/partition/DiskBackedPartitionStore.java > 110ce9d5520aa76763d925615aea34fafca1dc52 > giraph-core/src/main/java/org/apache/giraph/partition/Partition.java > b6b9551f20fbeb325b9033ea9fe79accc9f4e351 > giraph-core/src/main/java/org/apache/giraph/partition/PartitionStore.java > 763397e0f7e6b7d09c51b1679eed3097d2f89e01 > giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java > 0c1b40439a8891629b60aace4d89997ade42d1b5 > > giraph-core/src/main/java/org/apache/giraph/partition/SimplePartitionStore.java > ae17aac19001ccacc147287cc7a303d8cc17c91f > > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdMessages.java > 7e2b73b08be2a371b1dfe9118f5b1ca6f75b0d5e > giraph-core/src/main/java/org/apache/giraph/utils/ConfigurationUtils.java > 4bc4f4d5848590902894d35aad8c325177851604 > giraph-core/src/main/java/org/apache/giraph/utils/VertexIterator.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/utils/WritableUtils.java > 9163c08c6e4d089ec467f8a223efdaabef2bb995 > giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java > 112b76d03a234246ec9971f7f70be4b550ae5222 > giraph-core/src/test/java/org/apache/giraph/comm/RequestTest.java > 115c10839a03a01d15a53ccf3e90caeb0db498ee > > giraph-core/src/test/java/org/apache/giraph/comm/messages/TestIntFloatPrimitiveMessageStores.java > a8f6f70cdb59edf55e2eb441564e9c7ff5376ae5 > > giraph-core/src/test/java/org/apache/giraph/comm/messages/TestLongDoublePrimitiveMessageStores.java > 065926092bc886d680ad07849ad1b614e82b5bab > giraph-core/src/test/java/org/apache/giraph/io/TestEdgeInput.java > 45d946fbeeadc71213801ac33c20686bb782a109 > > giraph-core/src/test/java/org/apache/giraph/master/TestComputationCombinerTypes.java > b62775f1c7b3f561b5e7b05bb3ffb66ac42493c2 > giraph-core/src/test/java/org/apache/giraph/master/TestSwitchClasses.java > 6b0ed35391942f472fca43eeb773ad59715ffa7f > > giraph-core/src/test/java/org/apache/giraph/partition/TestPartitionStores.java > 0e68b56adac6a838cd314a169fdd9cdef1ca059f > giraph-examples/src/test/java/org/apache/giraph/TestBspBasic.java > 115da7e7a0e7c8c0ada15db5dc5b9e229e1d4290 > > giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsComputationTest.java > 7d326da9d5aa338aed87f9fddf5b292d17489329 > > giraph-examples/src/test/java/org/apache/giraph/examples/ConnectedComponentsComputationTestInMemory.java > dbcd569c216939a3fdd7bdddacf5871f500b8f28 > > giraph-examples/src/test/java/org/apache/giraph/examples/MinimumIntCombinerTest.java > 434c7561806fc099f077b526cff9ded1bdc3fa11 > > giraph-examples/src/test/java/org/apache/giraph/examples/TryMultiIpcBindingPortsTest.java > 1323ff6dda458c9589ab8e3e7a1173c7881a49c1 > > giraph-examples/src/test/java/org/apache/giraph/vertex/TestComputationTypes.java > 4f74fcb0ff835ee0fbfbf2cc822083c0bfbd0814 > > giraph-hive/src/main/java/org/apache/giraph/hive/input/vertex/examples/HiveIntIntNullVertex.java > PRE-CREATION > > giraph-hive/src/main/java/org/apache/giraph/hive/jython/HiveJythonUtils.java > 334f3828fedc5c077b9492e6f1a31a8e515082e4 > > giraph-hive/src/test/java/org/apache/giraph/hive/input/HiveVertexInputTest.java > c75652cc2f979736e1b8c72f88717f4ab0fe2e31 > > giraph-hive/src/test/resources/org/apache/giraph/jython/count-edges-launcher.py > a87f0309dce3f54332b76ff5239dffa3758c4224 > src/site/xdoc/quick_start.xml 5df6555e5d93bfae173b42bee4bbeaaf36366d1c > > Diff: https://reviews.apache.org/r/14527/diff/ > > > Testing > ------- > > Passes all new unittests > mvn clean verify > > > Thanks, > > Avery Ching > >
