relocate method in BufferAggregator. #4071

akashdw · 2017-03-17T02:39:23Z

Fixes #4026.
This pr introduces a new method relocate in BufferAggregator to allow relocation of any cached object.

leventov · 2017-03-17T04:44:02Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+
+  private NativeMemory getNativeMemory(ByteBuffer buffer)
+  {
+    NativeMemory nm = nmCache.get(buffer);


computeIfAbsent() should be used here. See #4034

Is this something agreed upon? Also, please explain advantages of computeIfAbsent() compared to this in this case.

More readable, intent is clearer: computeIfAbsent speaks for itself, harder to make a mistake.

One operation with Map instead of two separate operations.

Its a small method, readability and intent is pretty much clear.
Also, This is not a hot method and for now I will stick to the current implementation.

It's not a huge improvement, but it is an improvement by any metric - computeIfAbsent is clearer, shorter, safer, more efficient and the "right" way to perform this kind of task in Java 8+ world. I don't see reasons not to use it in completely new code.

leventov · 2017-03-17T04:44:28Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

  private final Map<Integer, Union> unions = new HashMap<>(); //position in BB -> Union Object
+  private IdentityHashMap<ByteBuffer, NativeMemory> nmCache = new IdentityHashMap<>();


Make nmCache final

leventov · 2017-03-17T04:52:31Z

processing/src/main/java/io/druid/query/aggregation/BufferAggregator.java

+   * @param newPosition  new position of an item in new ByteBuffer.
+   * @param newBuffer    ByteBuffer to be used.
+   */
+  default void relocate(int oldPostition, int newPosition, ByteBuffer newBuffer)


What is "old buffer"? What exactly does "Relocates any cached objects." mean? How this method could be sensibly defined for any other BufferAggregator, except SketchBufferAggregator?

I start understanding this vaguely after 15 mins of starring at this code, but the description should be more elaborate, and contain examples of code.

Might be more efficient to pass to this method newBuffer and two parallel sequences (lists, iterators, suppliers, whatever) of oldPositions and newPositions. This would allow to optimize relocate() implementation in SketchBufferAggregator specifically, because it won't access nmCache on each update.

It will be difficult to put BufferGrouper functionality as a code comment in BufferAggregator, I'll try to put more details without bringing in the BufferGrouper complexities.
I disagree with putting examples in java doc and also don't see other methods doing it.

Relocate is not a hot method(only called when BufferGrouper grows) and I think keeping the existing structure should be okay.

leventov · 2017-03-17T04:55:52Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+    Memory mem = new MemoryRegion(nm, newPosition, maxIntermediateSize);
+    Union newUnion = (Union) SetOperation.wrap(mem);
+
+    Union union = unions.get(oldPosition);


Could be just

unions.remove(oldPosition); unions.put(newPosition, newUnion);

leventov · 2017-03-17T04:57:06Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

 import java.util.Map;

 public class SketchBufferAggregator implements BufferAggregator
 {
  private final ObjectColumnSelector selector;
  private final int size;
  private final int maxIntermediateSize;
-
-  private NativeMemory nm;
-
  private final Map<Integer, Union> unions = new HashMap<>(); //position in BB -> Union Object


Int2ObjectMap could be used

This field was already getting used, not changing the existing fields for now. May be in a separate PR.

"Separate PR" will never happen, in practice. Boxed collections are not going to be replaced with primitive specializations in a single giant PR, we are replacing them along the way, one-by-one.

As I mentioned earlier, this field was already there, and this pr is not to refactor SketchBufferAggregator but fixing a bug exists in druid 10. It will be nice if you can provide some good reasons why it has to be changed now.

@akashdw this is a simple change and would actually improve performance in this case, so I would +1 this one

Happy to change it, could someone explain the benefits. @leventov Next time could you please provide reasoning for your suggestions b/c I have to spend more time researching.

leventov · 2017-03-17T05:00:15Z

...ava/io/druid/query/aggregation/datasketches/theta/BufferGrouperTestForSketchAggregation.java

+
+import java.nio.ByteBuffer;
+
+public class BufferGrouperTestForSketchAggregation


Please make the test class name end with "Test". There are some environments where the tests could be skipped otherwise (IntelliJ?)

himanshug · 2017-03-17T19:48:13Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

  private final Map<Integer, Union> unions = new HashMap<>(); //position in BB -> Union Object
+  private final IdentityHashMap<ByteBuffer, NativeMemory> nmCache = new IdentityHashMap<>();


I don't think we need to maintain this map... at one time we should only have one buffer to work with.

True only if relocate gets a batch update.

we need to keep this map b/c BufferAggregator does not guarantee ByteBuffer passed in any of the methods aggregate, relocate is going to be the same. There might be some implementations which can send different ByteBuffer and to support that we need to cache those ByteBuffers.
Given that, I will use the original interface relocate(int oldPosition, int new Position, ByteBuffer newBuffer) b/c diff relocate calls can get diff buffer's.

himanshug · 2017-03-17T19:52:01Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

@@ -113,4 +110,28 @@ public void close()
    unions.clear();
  }

+  @Override
+  public void relocate(int oldPosition, int newPosition, ByteBuffer newBuffer)


I think this method should berelocate(int[] newPositions, ByteBuffer newBuffer) and implementation should discard all old unions and build new ones for each new position and store in the unions map

something like...

void relocate(int[] newPositions, ByteBuffer newBuffer) { unions.clear(); nm = new NativeMemory(newBuffer); for (int i = 0; i < newPositions.length; i++) { Memory region = new MemoryRegion(...); Union union = wrap; unions.put(newPositions[i], union); } }

I disagree with relocate(int[] newPositions, ByteBuffer newBuffer) interface b/c we still need old positions to dereference old Union built on top of old ByteBuffer stored in unions map

had a conversation offline with @akashdw to clarify and agree on this change

Wouldn't relocate(int[] newPositions, ByteBuffer newBuffer) make what @cheddar wanted to do with quantiles (see #4026 (comment)) impossible?

In that proposal, datasketches quantiles aggregator wouldn't just be caching things that could be recomputed, it would be storing state that needs to be preserved.

Keeping the original interface relocate(int oldPosition, int new Position, ByteBuffer newBuffer) b/c of #4071 (comment)

himanshug · 2017-03-17T19:57:19Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

-    }
-
-    Memory mem = new MemoryRegion(nm, position, maxIntermediateSize);
+    NativeMemory nativeMemory = getNativeMemory(buf);


these changes are not needed

True only if relocate gets a batch update.

jihoonson · 2017-03-18T00:48:39Z

...query/aggregation/datasketches/theta/BufferGrouperUsingSketchMergeAggregatorFactoryTest.java

+        0.75f,
+        initialBuckets
+    );
+    grouper.init();


nit: grouper need to be closed after test.

jihoonson · 2017-03-18T00:56:27Z

processing/src/test/java/io/druid/query/aggregation/AggregationTestHelper.java

+  {
+    T[] results = (T[]) Array.newInstance(clazz, 2);
+    BufferAggregator agg = factory.factorizeBuffered(selector);
+    ByteBuffer myBuf = ByteBuffer.allocate(10040902);


The numbers used in this method seem to be something meaningful. Would you explain if they are?

These are random numbers.

gianm · 2017-03-21T07:33:39Z

processing/src/main/java/io/druid/query/groupby/epinephelinae/BufferGrouper.java

+    List<Map<Integer, Integer>> reloationMaps = new ArrayList<>(aggregators.length);
+    for (int i = 0; i < aggregators.length ; i++)
+    {
+      reloationMaps.add(new HashMap<Integer, Integer>(buckets));


This allocates an on-heap hashmap equal to the size of the off-heap table, which spoils the effort to keep memory usage offheap in groupBy v2. That's especially true for smaller bucket sizes (primitives rather than sketches) where this relocation map might be even larger than the off-heap table.

Please avoid creating relocationMaps, you should instead be able to call relocate on the BufferAggregator as entries are moved around.

ok, but then BufferAggregator will assume that no position collisions can happen.

but i see the point about keeping most memory usage to be off-heap.

@akashdw let us do the non-batching version then.

At least in groupby, the query code will always be growing by relocating data from the old tableBuffer to the new tableBuffer, so collisions shouldn't be an issue.

gianm · 2017-03-21T16:33:18Z

processing/src/main/java/io/druid/query/aggregation/BufferAggregator.java

+   * @param newBuffer    new aggregation buffer.
+   * @param newBuffer    new aggregation buffer.
+   */
+  default void relocate(int oldPosition, int newPosition, ByteBuffer newBuffer, ByteBuffer oldBuffer)


The order of args is a little weird here, old is first for position but last for buffer.

gianm · 2017-03-21T16:33:23Z

processing/src/main/java/io/druid/query/aggregation/BufferAggregator.java

+   * @param oldPosition old position of a cached object before aggregation buffer relocates to a new ByteBuffer.
+   * @param newPosition  new position of a cached object after aggregation buffer relocates to a new ByteBuffer.
+   * @param newBuffer    new aggregation buffer.
+   * @param newBuffer    new aggregation buffer.


Copy/paste error in docs.

gianm · 2017-03-21T16:34:12Z

processing/src/main/java/io/druid/query/aggregation/BufferAggregator.java

@@ -24,6 +24,7 @@
 import io.druid.query.monomorphicprocessing.RuntimeShapeInspector;

 import java.nio.ByteBuffer;
+import java.util.Map;


Import looks unnecessary.

gianm · 2017-03-21T16:43:41Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

@@ -87,15 +81,26 @@ public Object get(ByteBuffer buf, int position)
  //Note that this is not threadsafe and I don't think it needs to be
  private Union getUnion(ByteBuffer buf, int position)
  {
-    Union union = unions.get(position);
+    Union union = unions.get(buf) != null ? unions.get(buf).get(position) : null;


In the common case (union already exists) this will unnecessarily call unions.get(buf) twice.

gianm · 2017-03-21T16:45:47Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+  @Override
+  public void relocate(int oldPosition, int newPosition, ByteBuffer newBuffer, ByteBuffer oldBuffer)
+  {
+    createNewUnion(newBuffer, newPosition, true);


Would be good to also remove the old union so GC can reclaim the memory. And when a ByteBuffer has no more cached unions, you could remove the NativeMemory and Int2ObjectMap for it.

himanshug · 2017-03-21T16:52:54Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+  @Override
+  public void relocate(int oldPosition, int newPosition, ByteBuffer newBuffer, ByteBuffer oldBuffer)
+  {
+    createNewUnion(newBuffer, newPosition, true);


we need to get rid of old Union object and possibly oldBuffer from the cache (if this was the last relocate call)

himanshug · 2017-03-21T20:18:44Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

    }
+    unionMap.put(position, union);
+    unions.put(buf, unionMap);


this should be moved after line 101

himanshug · 2017-03-21T20:56:37Z

👍

leventov · 2017-03-21T21:03:26Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+
+  private NativeMemory getNativeMemory(ByteBuffer buffer)
+  {
+    NativeMemory nm = nmCache.get(buffer);


It's not a huge improvement, but it is an improvement by any metric - computeIfAbsent is clearer, shorter, safer, more efficient and the "right" way to perform this kind of task in Java 8+ world. I don't see reasons not to use it in completely new code.

leventov · 2017-03-21T21:05:33Z

...ches/src/main/java/io/druid/query/aggregation/datasketches/theta/SketchBufferAggregator.java

+    Union union = isWrapped
+                  ? (Union) SetOperation.wrap(mem)
+                  : (Union) SetOperation.builder().initMemory(mem).build(size, Family.UNION);
+    Int2ObjectMap<Union> unionMap = unions.get(buf);


unions.computeIfAbsent(buf, Int2ObjectOpenHashMap::new).put(position, union) could be used here

computeIfAbsent() require creating new function on each call and I'm not sure if it would impact performance.

No. In both places where I propose computeIfAbsent() non-capturing lambdas are required, they create only one instance in the JVM: http://stackoverflow.com/questions/27524445/does-a-lambda-expression-create-an-object-on-the-heap-every-time-its-executed#comment43488801_27524445

I did one small performance test, https://gist.github.com/akashdw/9f9b9db11fca602e562ecb47ec3ea24f,

Benchmark Mode Cnt Score Error Units
ComputeIfAbsent.withComputeIfAbsent avgt 25 150.218 ± 4.617 us/op
ComputeIfAbsent.withoutComputeIfAbsent avgt 25 113.956 ± 2.037 us/op

@akashdw have you even tried to analyze the results? Or you supposed that I should do this for you?

This benchmark is doing computeIfAbsent() against Int2ObjectOpenHashMap rather than IdentityHashMap, like in the PR

Default @Setup level is Level.Trial, that makes little sense for such code. Should be Level.Invocation.

The actual reason of the difference, is that Int2ObjectOpenHashMap::new takes the key as capacity of the map. It is a bug in the code that I suggested. It should be k -> new Int2ObjectOpenHashMap().

So the results of this benchmark: https://gist.github.com/leventov/2dabf3563e50012493e030aa69a40be4

ComputeIfAbsent.withComputeIfAbsent avgt 10 83.784 ± 1.476 us/op ComputeIfAbsent.withoutComputeIfAbsent avgt 10 85.201 ± 1.083 us/op

I. e. versions perform identically, that is just as expected, because IdentityHashMap actually doesn't override computeIfAbsent(). The default implementation of computeIfAbsent() does exactly the same calls as you do in the hand-written version. However

IdentityHashMap may specialize computeIfAbsent() in the future versions of JDK;

At some point of time unions and nmCache could be changed to use e. g. HashMap, which specializes computeIfAbsent(), and the code that already uses computeIfAbsent() will automatically improve;

All other advantages of computeIfAbsent() are still in place.

Note: -server is the default option with JDK 8, you don't need to specify it explicitly.

@gianm Yes, SketchAggregator.updateUnion is expected to take longer than getUnion and requested optimization might not yield performance difference.
If BufferAggregators called in the order you mentioned in #4071 (comment), then nmCache and unions will contain only one key.

whether or not this optimization is necessary right now.

I think the current changes are in good state, no optimization required and are ready to be merged.

However instead of addressing my original small code quality comment (from last week) that would have taken less than 5 min, @akashdw started a dispute and then a super time-draining benchmark battle

@leventov Are you suggesting to accept change request without doing validation?

To be very clear, one can not simply accept these kind of change requests without verification and validation b/c its about performance/correctness and not about changes you requested which can be done in 5 minutes.

fyi your initial recommendation had bug (b/c probably you didn't validated or benchmarked it )and caught during benchmarking.
Given,SketchAggregator.updateUnion is expected to take longer than getUnion and requested optimization might not yield performance difference, can you please unblock this PR ?

@akashdw

You started to do "performance validation", while taken down earlier suggestion by @himanshug (your coworker, btw), which was much better, both from performance and simplicity points of view, than both versions of code we were arguing about. When I paid attention to that dialogue between you and @himanshug, I realised immediately that the reasons why you decided it is impossible to implement @himanshug's suggestion are not valid and that suggestion is eligible.

So instead of spending a few mins thinking about @himanshug's suggestion, that you should have done if you cared about getUnion()'s performance, because the suggestion obviously makes getUnion() much more faster, you preferred to spend a significant time preparing invalid benchmark and throwing it at me, forcing me to spend a lot of time analyzing it (a required step when doing any benchmarking).

This is a terrible waste of yours and my time and not how communication in Druid PRs is expected to be done.

fyi your initial recommendation had bug (b/c probably you never validated or benchmarked it )and caught during benchmarking

Yes, it's my mistake, and I noted for myself for the future to pay more attention when leaving PR review comments. However bugs are unavoidable in code completely. The fact that you didn't notice it before starting benchmarking also tells that you don't validate review suggestions functionally very attentively.

The fact that benchmark helped to find the mistake is a total coincidence and doesn't mean that benchmarking was useful.

Unblocking PR in it's current state means that I'm taking full responsibility for this situation, that I'm still not going to do. (But it doesn't mean that I impose that it's fully your responsibility, no.) I learned some lessons from this situation and I suggest you to do the same. So currently I suggest you to implement @himanshug / my suggestion. I think it's "compromise enough" because it's not what we were arguing about. Let alone it's just better.

@leventov BufferAggregator interface does not tell about the order of execution mentioned here #4071 (comment), now BufferAggregators implementations have two options:

Favor correctness : i.e to support any execution order of BufferAggregator methods. (e.g support aggregate-related methods even without init() being called)

Favor performance : i.e assume relocate method in BufferAggregator. #4071 (comment) execution order and fail aggregate-related methods if methods are not called in correct order.

I choose correctness over performance and Its unfair to block someones PR who disagrees with your choice.

FYI: @himanshug already +1'ed this PR.

@gianm @leventov

The contract of BufferAggregator is that before aggregate(int pos, ByteBuffer buff) is called, caller ensures that init(pos,buff) has been called for that position and buffer.... and that is the end of it. Now, there may or may not be multiple ByteBuffers in the scene at a time .
However, given the current state of Druid, there is only one ByteBuffer there at a time and even groupBy-v2 follows the contract described at #4071 (comment) .
But BufferAggregator contract is more flexible than that. @akashdw made me realize that and to prevent future bugs from happening he stopped making that assumption in the code and created maps. and that is why I gave up on my suggestion and +1d it.

@akashdw @himanshug this is BDUF. Possible redesign of BufferGrouper which won't include "rehash" will make relocate() obsolete as well, so rework of SketchBufferAggregator will be needed anyway. And this BDUF not really safe and couldn't easily be made safe, as I shown above. However both versions are correct now.

@akashdw didn't you notice that you are the Druid contributor who is the most reluctant to making changes suggested by (any) reviewers? General mindset among Druid committers is implementing reviewer's comments or compromise decisions to achieve agreement without extra communication, with high level of common acceptance of the code. Your mindset seems to be to persuade reviewers that your original code is the best. Moreover, your way of persuading is highly disrespectful to reviewer's (my) time.

The shortest and least time-consuming way to resolve this situation now for all parties is for you to spend 15 minutes and implement expectedBuffer and throw away maps. It should be clear for you from the yesterday's evening. Continuing to argue and draining time is entirely your choice.

cheddar · 2017-03-22T19:25:01Z

@gianm regarding your questions about the contract. I agree that what you did in groupBy v2 is totally valid and 100%, totally according to contract.

The basic contract is really simply described by the function signatures and that is "you get a ByteBuffer and a position, use the position on the ByteBuffer and your bytes will be there". There is then the other one of "an area of memory is always initialized before being aggregated into", that one isn't called out in the docs as well.

gianm · 2017-03-22T20:50:06Z

@leventov @akashdw I'm sorry to see such an argument between two people that clearly have strong feelings about Druid!

My vote is to defer to @akashdw's choice for 0.10.0 assuming it is acceptable to committers that are using DataSketches in production (like @himanshug, @cheddar). But I would also like to open an issue for the future to clarify the BufferAggregator contract — it sounds like it's pretty confusing what it allows and doesn't allow — and then use that clarified contract to optimize SketchBufferAggregator more after 0.10.0.

@akashdw @himanshug this is BDUF. Possible redesign of BufferGrouper which won't include "rehash" will make relocate() obsolete as well, so rework of SketchBufferAggregator will be needed anyway.

I don't think it's BDUF, since Query and BufferAggregator are both valid extension points for Druid extensions, and we can't assume we can control all possible implementations. We need the interfaces to be clear about what is allowed and disallowed, and we shouldn't write code that violates them, or else users' site specific custom Queries and Aggregators might not work properly.

Even future code written as part of Druid might not work properly, after we all put this behind us and forget that SketchBufferAggregator doesn't quite follow the contract 100%. This is actually how we got into this situation in the first place! When I wrote groupBy v2, I used BufferAggregator interface in a way that other queries hadn't done before, which I thought satisfied the contract, but broke SketchBufferAggregator. Arguably, the old version of SketchBufferAggregator didn't follow the contract, but it was "OK" until the change to groupBy.

So, I think the maps are useful and it's valid choice to be a little "flexible" until the BufferAggregator interface is clarified.

gianm

LGTM

@akashdw have you had a chance to run this in prod to verify it fixes the original issue #4026?

gianm · 2017-03-23T15:43:06Z

@leventov I understand this patch may not be perfect but do you see show-stopper issues that we cannot resolve in future releases?

akashdw · 2017-03-23T16:49:29Z

@gianm testing this patch in one of the production environment, will let you know once done with the testing.

gianm · 2017-03-23T17:20:57Z

Thanks @akashdw, please keep us posted.

akashdw · 2017-03-23T19:47:17Z

@gianm tested this patch in one of our environment and I don't see relocate issue with GroupByV2 anymore.

gianm · 2017-03-23T20:07:54Z

Spoke with @leventov offline. I'll merge this since it has +1s from me and @himanshug.

akashdw mentioned this pull request Mar 17, 2017

groupBy v2 failing intermittently with complex columns #4026

Closed

akashdw closed this Mar 17, 2017

akashdw reopened this Mar 17, 2017

leventov requested changes Mar 17, 2017

View reviewed changes

gianm added this to the 0.10.0 milestone Mar 17, 2017

gianm added the Improvement label Mar 17, 2017

himanshug reviewed Mar 17, 2017

View reviewed changes

akashdw added 3 commits March 17, 2017 13:58

relocate method in BufferAggregator.

dc37f05

Unused import.

a0d7a0b

Detailed javadoc.

a24e3a3

jihoonson reviewed Mar 18, 2017

View reviewed changes

using Int2ObjectMap.

34da4fe

akashdw force-pushed the sketchFix branch from 1ad5ee1 to 34da4fe Compare March 18, 2017 01:25

batch relocate.

ef92b17

gianm requested changes Mar 21, 2017

View reviewed changes

akashdw added 2 commits March 21, 2017 09:29

Revert batch relocate.

078f3d1

Unused import.

fd1a77f

gianm reviewed Mar 21, 2017

View reviewed changes

himanshug reviewed Mar 21, 2017

View reviewed changes

code comments.

a6b4f70

himanshug reviewed Mar 21, 2017

View reviewed changes

code comment.

e9a3193

leventov requested changes Mar 21, 2017

View reviewed changes

gianm approved these changes Mar 23, 2017

View reviewed changes

gianm merged commit ff7f90b into apache:master Mar 23, 2017

gianm pushed a commit to gianm/druid that referenced this pull request Mar 23, 2017

relocate method in BufferAggregator. (apache#4071)

5508d07

gianm mentioned this pull request Mar 23, 2017

[Backport] relocate method in BufferAggregator. #4104

Merged

fjy pushed a commit that referenced this pull request Mar 23, 2017

relocate method in BufferAggregator. (#4071) (#4104)

0313128

gianm mentioned this pull request Mar 24, 2017

Druid 0.10.0 release notes #3944

Closed

jon-wei mentioned this pull request Mar 29, 2017

Support limit push down for GroupBy #3873

Merged

		private final Map<Integer, Union> unions = new HashMap<>(); //position in BB -> Union Object
		private IdentityHashMap<ByteBuffer, NativeMemory> nmCache = new IdentityHashMap<>();


		import java.nio.ByteBuffer;

		public class BufferGrouperTestForSketchAggregation

		private final Map<Integer, Union> unions = new HashMap<>(); //position in BB -> Union Object
		private final IdentityHashMap<ByteBuffer, NativeMemory> nmCache = new IdentityHashMap<>();

relocate method in BufferAggregator. #4071

relocate method in BufferAggregator. #4071

Conversation

akashdw commented Mar 17, 2017

Choose a reason for hiding this comment

akashdw Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashdw Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashdw Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashdw Mar 18, 2017 • edited Loading

Choose a reason for hiding this comment

himanshug Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

himanshug Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanshug commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashdw Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashdw Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

himanshug Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheddar commented Mar 22, 2017

gianm commented Mar 22, 2017

gianm left a comment

Choose a reason for hiding this comment

gianm commented Mar 23, 2017

akashdw commented Mar 23, 2017

gianm commented Mar 23, 2017

akashdw commented Mar 23, 2017

gianm commented Mar 23, 2017

akashdw Mar 17, 2017 •

edited

Loading

akashdw Mar 17, 2017 •

edited

Loading

akashdw Mar 17, 2017 •

edited

Loading

akashdw Mar 18, 2017 •

edited

Loading

himanshug Mar 17, 2017 •

edited

Loading

himanshug Mar 17, 2017 •

edited

Loading

akashdw Mar 22, 2017 •

edited

Loading

akashdw Mar 22, 2017 •

edited

Loading

himanshug Mar 22, 2017 •

edited

Loading