Build JNI IFA switching helpers as C calls #2044

NigelYiboYu · 2018-05-31T21:35:08Z

The JNI IFA switching on and off helpers use transition assembly sequences
that saves and restores all GPRs, all FPRs and volatile VRFs. This is slow
and unnecessary.

This commit changes builds IFA calls as C function calls, which saves only
the assigned registers.

Signed-off-by: Nigel Yu [email protected]

NigelYiboYu · 2018-05-31T21:35:39Z

@fjeremic JNI IFA glue code removal commit here.
Marking this as WIP until more testing is done.

This change can cause merge conflict with this pull request. A manual conflict resolution is needed.

fjeremic · 2018-06-01T03:35:46Z

runtime/compiler/build/files/target/z.mk

@@ -66,6 +66,7 @@ JIT_PRODUCT_SOURCE_FILES+=\
    compiler/z/codegen/J9MemoryReference.cpp \
    compiler/z/codegen/J9S390CHelperLinkage.cpp \
    compiler/z/codegen/J9S390PrivateLinkage.cpp \
+    compiler/z/codegen/J9S390JNILinkage.cpp \


I think we'll need to modify the CMake builds as well.

I don't see CMake lists contain these cpp file names. It seems to operate on directories for z codegen.

fjeremic · 2018-06-01T03:46:47Z

runtime/compiler/runtime/Runtime.cpp

-JIT_HELPER(jitPreJNICallOffloadCheck);
-JIT_HELPER(jitPostJNICallOffloadCheck);
+JIT_HELPER(fast_jitPreJNICallOffloadCheck);
+JIT_HELPER(fast_jitPostJNICallOffloadCheck);


Is there a reason for deviation from the naming convention?

I changed this to use the same name as the VM helper name for consistency. Didn't realize that the JIT helpers have a naming convention. I'll change it back.

I'm keeping the fast_ in the names because these two lines are effectively extern c fast_* and referring to the VM helpers.

fjeremic · 2018-06-01T03:47:14Z

runtime/compiler/trj9/z/codegen/J9S390JNILinkage.hpp

@@ -0,0 +1,70 @@
+/*******************************************************************************
+ * Copyright (c) 2000, 2018 IBM Corp. and others


Copyright needs to be updated to be 2018, 2018. Similarly for the other file.

fjeremic · 2018-06-01T03:47:48Z

runtime/compiler/trj9/z/codegen/J9S390JNILinkage.hpp

+
+namespace TR {
+
+class J9S390JNILinkage : public TR::S390PrivateLinkage


We need to Doxygen document this entire class.

NigelYiboYu · 2018-06-08T16:19:38Z

The changes are tested OK. Ready for reviews.

NigelYiboYu · 2018-06-12T14:48:32Z

To illustrate the differences before and after this change, here's the trace files from a JNI benchmark where the same native function is invoked in a loop for millions of times.

Full trace files:
JNI-before.txt
JNI-after.txt

Annotated excerpt for them:
annotated JNI excerpt before the change.txt
annotated JNI excerpt after the change.txt

fjeremic · 2018-06-14T19:40:07Z

runtime/compiler/z/codegen/J9S390PrivateLinkage.cpp

@@ -49,6 +49,7 @@
 #include "z/codegen/TRSystemLinkage.hpp"
 #include "runtime/J9Profiler.hpp"
 #include "runtime/J9ValueProfiler.hpp"
+#include "z/codegen/J9S390CHelperLinkage.hpp"


Can we please place this with the other z/codegen includes in alphabetical order?

fjeremic · 2018-06-14T20:08:00Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

+#endif
+
+TR::Register *
+TR::S390CHelperLinkage::buildIndirectDispatch(TR::Node * callNode,


This function seems built very specifically for JNI IFA. Does it even belong here? There are also a handful of booleans below which can be folded away. I'm questioning the nature of the existence of this API. CHelper functions should all be statically addressable. Why exactly do we need indirect dispatch for them?

#1 Admittedly this API is very customized towards the JNI IFA calls because IFA is the only consumer of this. Some of the booleans (isFastPathOnly and even the #if defined(J9ZOS390) for instance) can be folded away. I kept them there in case future users may need them. The IFA calls are zOS only but I figured that we can build indirect helper calls for other helpers on Linux.

#2 Also, the c function fast_jitPost(pre)JNICallOffloadCheck can't be addressed statically as per current design. Direct invocation will result in VM assert failures. I suspect all function pointers in the JITConfig structure are not directly invoke-able. By directly invoke-able, I mean BRASL jump to the fast_jitPre(post)JNICallOffloadCheck function pointers directly.

I didn't use callNode->getSymbolReference()->getSymbol()->castToMethodSymbol()->getMethodAddress(); Will try and see how this is different from direct function pointer approach.

#3 One JNI IFA specific aspect is the register saving/restoring such as saveRegistersForIFAHelper(). This logic used to be baked inside the glue code is needed if CEEHDLR is enabled. CEE handlers can crash if sys SP and the vmthread registers are not saved on the native side before invoking the native. This has to be baked into the IFA call builder.

In conclusion:
#1 is simple to do. I can fold away the booleans.
For #2 we have to see why the C functions are not directly invoke-able using function pointers.
#3 has to be in the builder function, whether it be a CHelper function or a function in a different class.

fjeremic · 2018-06-14T20:08:41Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

+                                                     offsetof(J9JavaVM, jitConfig), cg()));
+
+
+   // get J9TR_JitConfig_fast_jitPreJNICallOffloadCheck from jit config


Isn't this a compile time constant? Why do we need these 3 indirections? i.e. how is this any different from fast_jitInstanceOf? Are we able to extract the address statically at compile time via:

callNode->getSymbolReference()->getSymbol()->castToMethodSymbol()->getMethodAddress();

Yes, they are compile time constants. The indirections are copied from the old glue code.

I had difficulty building direct BRASL calls to fast_jitPre(post)JNICallOffloadCheck. VM changes might be needed for us to jump directly to fast_jitPostJNICallOffloadCheck.

fjeremic · 2018-06-14T20:22:42Z

runtime/compiler/z/codegen/J9S390JNILinkage.cpp

+  * \endverbatim
+  */
+void
+TR::J9S390JNILinkage::setupJNICallOutFrame(TR::Node * callNode,


Is it possible to consolidate all TR::S390JNICallDataSnippet construction into this function? Why split between two functions. Can we change the return type to TR::S390JNICallDataSnippet* and construct the entire object here and return it?

Grepping this file for jniCallDataSnippet->set it seems construction of this object is split between 3 functions but I don't see a reason why it can't all be consolidated into a single place.

Will cleanup the S390JNICallDataSnippet and consolidate the code.

fjeremic · 2018-06-14T20:23:36Z

runtime/compiler/z/codegen/J9S390JNILinkage.cpp

+
+   // set up Java Thread
+   intptrj_t constJNICallOutFrameType = fej9->constJNICallOutFrameType();
+   TR_ASSERT( constJNICallOutFrameType < MAX_IMMEDIATE_VAL, "OMR::Z::Linkage::setupJNICallOutFrame constJNICallOutFrameType is too big for MVHI");


This assert needs fixing. What exactly is it guarding? Where is the MVHI?

Yeah, this is from the old code and should be deleted.

fjeremic · 2018-06-14T20:29:11Z

runtime/compiler/z/codegen/J9S390JNILinkage.cpp

+                          generateS390MemoryReference(jniCallDataSnippet->getBaseRegister(), jniCallDataSnippet->getPCOffset(), codeGen));
+
+   // store out jsp
+   generateRXInstruction(codeGen, TR::InstOpCode::getStoreOpCode(), callNode, tempReg,


Can we rename codeGen to cg to stay consistent with the rest of the codebase?

Changed to cg()

NigelYiboYu · 2018-07-09T16:10:29Z

@fjeremic This PR is ready.

fjeremic · 2018-07-11T16:13:28Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2018 IBM Corp. and others
+ * Copyright (c) 2017, 2018 IBM Corp. and others


Just leaving a note that this is a valid change as the file was introduced in 2017 originally, not 2000.

fjeremic · 2018-07-11T16:27:30Z

runtime/compiler/z/codegen/J9S390JNILinkage.cpp

+
+   TR::SymbolReference * offloadSymRef = cg()->symRefTab()->findOrCreateRuntimeHelper(helperIndex, false, false, false);
+   offloadSymRef->getSymbol()->setName(symName);
+   offloadSymRef->getSymbol()->castToMethodSymbol()->setMethodAddress(targetAddr);


I don't think we need this code as this is already handled when the helper is created here:

https://github.com/eclipse/omr/blob/70ff3c7d2a7fa2f85f380dadf23e5acda65a45bc/compiler/compile/OMRSymbolReferenceTable.cpp#L714-L735

Are we able to remove this and simply pass in the TR_RuntimeHelper index instead of bool isPreJNIHelper? We can also remove the above two extern "C" definitions and simplify this function greatly. In fact it may become so small as to not even warrant an existence?

Removed the extern statements and used TR_RuntimeHelper indices instead.

NigelYiboYu · 2018-07-19T15:43:03Z

After refactoring the J9S390JNILinkage and J9S390CHelperLinkage classes, I still need to introduce two things to J9S390CHelperLinkage:

the stack adjustment for JNI IFA only, and
the loading of an environment. I believe the environment should be part of the CHelper because the XPLINK has it as an optional thing in GPR5 which is probably not used often. In the case of JNI IFA helper call, the environment is needed for proper functioning.

fjeremic · 2018-07-20T15:19:23Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

+         cursor = generateRILInstruction(cg(), TR::InstOpCode::IIHF, callNode, javaStackPointerRegister, envHi);
+         cursor = generateRILInstruction(cg(), TR::InstOpCode::IILF, callNode, javaStackPointerRegister, envLo);
+         }
+      }


This whole sequence should simply use genLoadAddressConstant and pass the environment constant through. The function will figure out the best sequence to generate.

@NigelYiboYu Loading environment register is necessary for calling a C function from JIT code. I omitted that part from Linkage as Assembly listing for fast_jitInstanceOf was using it as volatile and we overwrite the register so anyway loaded value was not used. I checked other dual mode helper, Some of them returns the function pointer of the slow path helper function which belongs to same library and in that scenario environment register is needed to get the function pointer which is currently taken care by assembly glue in znathelp.m4. That is the only use I could find currently.
Correct change here would be respecting XPLINK linkage and also pass environment register for instanceOf as well. Means before calling fastPath helper, environment register should be loaded.

fjeremic · 2018-07-20T15:24:28Z

runtime/compiler/z/codegen/J9S390JNILinkage.cpp

+extern "C" void fast_jitPostJNICallOffloadCheck();
+
+void
+TR::J9S390JNILinkage::callIFAOffloadCheck(bool isPreJNIHelper, bool isJNICallOutFrame)


We should fold this function away. I don't think it's needed. The environment is already present via TR_S390CEnvironmentAddress so there is no need to deal with the function pointers etc. which renders this function an effective three liner:

TR::SymbolReference * offloadSymRef = cg()->symRefTab()->findOrCreateRuntimeHelper(helperIndex, false, false, false); TR::Node* helperCallNode = TR::Node::createWithSymRef(TR::call, 0, offloadSymRef); cHelperLinkage->buildDirectDispatch(helperCallNode, NULL, NULL, true, isJNICallOutFrame, reinterpret_cast<intptrj_t>(TR_S390CEnvironmentAddress));

I'm not sure it has a need for existence.

r30shah · 2018-07-23T18:25:04Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

+         cursor = generateRILInstruction(cg(), TR::InstOpCode::IIHF, callNode, javaStackPointerRegister, envHi);
+         cursor = generateRILInstruction(cg(), TR::InstOpCode::IILF, callNode, javaStackPointerRegister, envLo);
+         }
+      }


@NigelYiboYu Loading environment register is necessary for calling a C function from JIT code. I omitted that part from Linkage as Assembly listing for fast_jitInstanceOf was using it as volatile and we overwrite the register so anyway loaded value was not used. I checked other dual mode helper, Some of them returns the function pointer of the slow path helper function which belongs to same library and in that scenario environment register is needed to get the function pointer which is currently taken care by assembly glue in znathelp.m4. That is the only use I could find currently.
Correct change here would be respecting XPLINK linkage and also pass environment register for instanceOf as well. Means before calling fastPath helper, environment register should be loaded.

fjeremic · 2018-07-31T20:27:56Z

runtime/oti/VMHelpers.hpp

@@ -1133,7 +1133,7 @@ class VM_VMHelpers
 	 * @param[in] currentThread the current J9VMThread
 	 */
 	static VMINLINE void
-	beforeJNICall(J9VMThread *currentThread)
+	beforeJNICall(J9VMThread *currentThread, J9Method* method)


Can we update the documentation for this newly added parameter?

Added the new param to documentation.

fjeremic · 2018-07-31T20:35:47Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

   {
   RealRegisterManager RealRegisters(cg());
   bool isHelperCallWithinICF = deps != NULL;
-   // TODO: Currently only jitInstanceOf is fast path helper. Need to modify following condition if we add support for other fast path only helpers
-   bool isFastPathOnly = callNode->getOpCodeValue() == TR::instanceof;


Could we leave this TODO around? This is definitely something we still want to do once we refactor the runtime helpers into a sane table with properties.

TODO is added back

fjeremic · 2018-07-31T20:36:35Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.hpp

+                                     TR::RegisterDependencyConditions** deps,
+                                     TR::Register *returnReg=NULL,
+                                     bool forceFastPath = false,
+                                     TR_RuntimeHelper helperIndex = TR_S390numRuntimeHelpers);


Could we move the documentation here such that it is with the definition of the API and document foreFastPath as well?

Documentation is moved to the hpp file with forceFastPath added.

fjeremic · 2018-07-31T20:38:11Z

@r30shah anything else you'd like to review on this one? I still see an "x" beside your review but GitHub can't seem to find what it was referencing.

fjeremic · 2018-07-31T20:55:18Z

Jenkins test sanity

r30shah · 2018-07-31T21:38:22Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

@@ -364,6 +361,17 @@ TR::Register * TR::S390CHelperLinkage::buildDirectDispatch(TR::Node * callNode,
      }
   TR::SymbolReference * callSymRef = callNode->getSymbolReference();
   void * destAddr = callNode->getSymbolReference()->getSymbol()->castToMethodSymbol()->getMethodAddress();
+
+#if defined(J9ZOS390)


@NigelYiboYu I think my previous comment was for this section. For now we need this part for calling out helpers written in C that means we need to load environment for instanceOf as well. Is that covered? Looking through your other changes.
I am going through your changes now.

@fjeremic I am thinking of moving some of the context switching code from helper Glue code to in body and see the effect. I expect a small effect on the method footprint. If the effect is not much then we can move this inlined. I believe x86 already do this. We can make our helper linkage more generic this way.
Though it is something we can do later, should not affect this PR.

@r30shah let's give it a try and see if it has any negative effects on performance.

InstanceOf is not covered in this PR. This whole pull request is for JNI and IFA switching. We can come back and change instanceOf helper to load environment.

Actually good point @r30shah. Thinking about this again we have duplicate information here. The helper index is already stored in callNode->getSymbolReference()->getReferenceNumber() so we shouldn't need to pass it as an argument to the CHelper linkage buildDirectDispatch.

This information is already available through the callNode. @NigelYiboYu are we able to get rid of this extra parameter? Once this is done we can remove the below check for if (helperIndex != TR_S390numRuntimeHelpers) since a helper index must always be available. This will fix the instanceOf helper as well.

The extra helper index is removed.

Update:

Environment pointer is now only loaded if the C helper is fast-path. Non-fastPath helpers jump to glue code, which does its own environment handling.

r30shah · 2018-07-31T21:58:16Z

runtime/compiler/z/codegen/J9S390CHelperLinkage.cpp

+
+#if defined(J9ZOS390)
+   // TR_S390numRuntimeHelpers represents invalid helper index
+   if (helperIndex != TR_S390numRuntimeHelpers)


@NigelYiboYu I see that a declaration of this function defines helperIndex with TR_S390numRuntimeHelpers and for JNI linkage calls (for pre/post offload check) , this function (buildDirectDispatch) will be called with TR_RuntimeHelper equal to TR_S390jitPreJNICallOffloadCheck or TR_S390jitPostJNICallOffloadCheck. Is it correct?

Yes, JNI IFA uses TR_S390jitPreJNICallOffloadCheck and TR_S390jitPostJNICallOffloadCheck.
For the lack of a better choice, I use TR_S390numRuntimeHelpers to validate the helper index parameter.

Okk. I think better choice here would be use of this isFastPathOnly. I think we only set this when we are calling a C helper function directly from JIT'd code. This should also cover the case for instanceOf and load environment register for that.

The JNI IFA switching on and off helpers use transition assembly sequences that saves and restores all GPRs, all FPRs and volatile VRFs. This is slow and unnecessary. This commit changes builds IFA calls as C function calls, which saves only the assigned registers. Signed-off-by: Nigel Yu <[email protected]>

fjeremic · 2018-08-01T18:13:39Z

@r30shah could you please review again and see if all your concerns were addressed?

r30shah

@NigelYiboYu @fjeremic Looks good to me.

fjeremic · 2018-08-01T21:09:51Z

Jenkins test sanity

gacholio · 2018-08-01T21:26:18Z

@fjeremic - you really need to stop this shotgun approach to launching tests, especially considering that the changes in this PR don't affect any of the platforms in PR testing.

fjeremic · 2018-08-01T21:36:33Z

@fjeremic - you really need to stop this shotgun approach to launching tests, especially considering that the changes in this PR don't affect any of the platforms in PR testing.

@gacholio is there a reason you say this? This change modified cnathelp.cpp which gets compiled and runs on all platforms. I've been bitten before by not fully testing changes before merging. Irregardless though IMO the commiters should be running tests on any change before it is merged.

Edit:

If we are stressing the farm by launching so many builds I think that should be a resource issue. We need proper testing before changes are merged and especially in a world where intermittent problems with JIT compilations are a thing I would rather be safe than sorry having to debug user issues in the field once bugs escape.

gacholio · 2018-08-01T21:51:53Z

I say this because I've been waiting for a single machine compile for hours, no doubt due to too much stuff being launched. It most certainly is a resource issue as well, but that's unlikely to be fixed soon (if ever). Even the simplest change takes days to get in.

Also worth noting that running 8/9/10/11 testing is completely unnecessary on common code.

fjeremic · 2018-08-01T21:55:03Z

I say this because I've been waiting for a single machine compile for hours, no doubt due to too much stuff being launched. It most certainly is a resource issue as well, but that's unlikely to be fixed soon (if ever). Even the simplest change takes days to get in.

Also worth noting that running 8/9/10/11 testing is completely unnecessary on common code.

I see, so do you propose for this change running only JDK8 for example as the right way to go? I would still not feel comfortable merging a change which affects all platforms without running sanity on the said platforms.

I'm not sure what to say about the resource issue. We either need to have more granular tests or get more machines. I don't see a solution to this, and not running tests is not a solution that we should accept.

pshipton · 2018-08-01T21:58:41Z

We are working on getting more machines for some platforms, by sending hardware to UNB and helping to set it up. Hopefully by Sept.

fjeremic · 2018-08-01T22:00:45Z

@pshipton do we have guidelines for committers on how we should be launching testing? If we are unnecessarily stressing the farm too much we should come up with guidelines to minimize this.

Edit:

For example in OMR we have:
https://github.com/eclipse/omr/blob/master/doc/GuideForCommitters.md

If the change affects multiple platforms or 64-bit Windows, you must initiate a pull request build on all affected platforms prior to merging. ...

gacholio · 2018-08-01T22:00:53Z

Personally, I only ever run jdk8 with a single platform for things that are obviously common (which does occasionally result in problems on windows since the compiler is strict in ways no other one is).

My first proposal is that we eliminate the shotgun from jenkins entirely - one comment per build launch will make people consider what they're doing. It's just too easy to spam the world at the moment.

pshipton · 2018-08-02T00:49:38Z

do we have guidelines for committers on how we should be launching testing? If we are unnecessarily stressing the farm too much we should come up with guidelines to minimize this.

Not that I'm aware of. I think "test sanity" is the correct approach for JIT unless the change is specific to a platform, or generic across jdk versions. I expect JIT committers know best how to test JIT changes. Perhaps not all of xlinux, xlinux large heap, win and win-32 (jdk8 only) are always required since they are all x. Plus multiple jdk version may just duplicate testing depending on the change.

The automatic launching of the jdk11 builds is a mistake, since they don't work yet. I've opened #2539.

pshipton · 2018-08-02T00:56:24Z

My first proposal is that we eliminate the shotgun from jenkins entirely - one comment per build launch will make people consider what they're doing.

Works for me, at least for jdk versions. I've accidentally run multiple jdk versions when I didn't mean to.

fjeremic · 2018-08-02T01:45:56Z

My first proposal is that we eliminate the shotgun from jenkins entirely - one comment per build launch will make people consider what they're doing. It's just too easy to spam the world at the moment.

I agree this would be a step in the right direction.

fjeremic · 2018-08-02T14:43:19Z

JDK11 builds are not supposed to be enabled, see #2539. Windows failures due to #2129.

NigelYiboYu force-pushed the jni-call-prejni branch 2 times, most recently from f979881 to 39c57a0 Compare May 31, 2018 21:46

fjeremic added the comp:jit label Jun 1, 2018

fjeremic self-assigned this Jun 1, 2018

fjeremic reviewed Jun 1, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch from 39c57a0 to 4011d72 Compare June 8, 2018 16:15

NigelYiboYu changed the title ~~WIP: Build JNI IFA switching helpers as C calls~~ Build JNI IFA switching helpers as C calls Jun 8, 2018

NigelYiboYu force-pushed the jni-call-prejni branch from 4011d72 to 7f72e17 Compare June 13, 2018 14:52

fjeremic reviewed Jun 14, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch 4 times, most recently from fe9f781 to 44bad56 Compare July 9, 2018 14:51

fjeremic reviewed Jul 11, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch 5 times, most recently from ce1f370 to 818c68a Compare July 19, 2018 15:34

fjeremic reviewed Jul 20, 2018

View reviewed changes

r30shah requested changes Jul 23, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch from 818c68a to 867ddec Compare July 30, 2018 14:29

NigelYiboYu force-pushed the jni-call-prejni branch from 2ef9b02 to baf3ac6 Compare July 30, 2018 20:35

fjeremic reviewed Jul 31, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch from baf3ac6 to 5ee8d18 Compare July 31, 2018 20:49

fjeremic approved these changes Jul 31, 2018

View reviewed changes

r30shah requested changes Jul 31, 2018

View reviewed changes

NigelYiboYu force-pushed the jni-call-prejni branch from 5ee8d18 to b346a0b Compare August 1, 2018 15:53

NigelYiboYu force-pushed the jni-call-prejni branch from b346a0b to 27118b0 Compare August 1, 2018 18:08

r30shah approved these changes Aug 1, 2018

View reviewed changes

fjeremic approved these changes Aug 1, 2018

View reviewed changes

pshipton mentioned this pull request Aug 2, 2018

"jenkins test all" includes jdk11 but should not #2539

Closed

fjeremic merged commit b9e3505 into eclipse-openj9:master Aug 2, 2018

fjeremic mentioned this pull request Aug 13, 2018

Revert "Build JNI IFA switching helpers as C calls" #2595

Merged

		@@ -0,0 +1,70 @@
		/*******************************************************************************
		* Copyright (c) 2000, 2018 IBM Corp. and others


		namespace TR {

		class J9S390JNILinkage : public TR::S390PrivateLinkage

		offsetof(J9JavaVM, jitConfig), cg()));


		// get J9TR_JitConfig_fast_jitPreJNICallOffloadCheck from jit config

Build JNI IFA switching helpers as C calls #2044

Build JNI IFA switching helpers as C calls #2044

Conversation

NigelYiboYu commented May 31, 2018

NigelYiboYu commented May 31, 2018 • edited Loading

Choose a reason for hiding this comment

NigelYiboYu Jun 4, 2018 • 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

NigelYiboYu commented Jun 8, 2018

NigelYiboYu commented Jun 12, 2018 • 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

NigelYiboYu commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NigelYiboYu commented Jul 19, 2018 • 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

fjeremic commented Jul 31, 2018

fjeremic commented Jul 31, 2018

Choose a reason for hiding this comment

r30shah Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NigelYiboYu Aug 1, 2018 • 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

fjeremic commented Aug 1, 2018

r30shah left a comment • edited Loading

Choose a reason for hiding this comment

fjeremic commented Aug 1, 2018

gacholio commented Aug 1, 2018

fjeremic commented Aug 1, 2018 • edited Loading

gacholio commented Aug 1, 2018

fjeremic commented Aug 1, 2018

pshipton commented Aug 1, 2018

fjeremic commented Aug 1, 2018 • edited Loading

gacholio commented Aug 1, 2018

pshipton commented Aug 2, 2018 • edited Loading

pshipton commented Aug 2, 2018

fjeremic commented Aug 2, 2018

fjeremic commented Aug 2, 2018

NigelYiboYu commented May 31, 2018 •

edited

Loading

NigelYiboYu Jun 4, 2018 •

edited

Loading

NigelYiboYu commented Jun 12, 2018 •

edited

Loading

NigelYiboYu commented Jul 19, 2018 •

edited

Loading

r30shah Jul 31, 2018 •

edited

Loading

NigelYiboYu Aug 1, 2018 •

edited

Loading

r30shah left a comment •

edited

Loading

fjeremic commented Aug 1, 2018 •

edited

Loading

fjeremic commented Aug 1, 2018 •

edited

Loading

pshipton commented Aug 2, 2018 •

edited

Loading