[Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) #2245

mylinyuzhi · 2018-06-12T14:00:05Z

What do these changes do?

in java-worker, there are two ways to implement the ray.call: asm-rewrite and remote-lambda.
the asm-rewrite is not elegant and remote-lambda is too slow. And can not get the meta-information of the called function at runtime in the two solutions. e.g. @rayremote(xxx=yyy), at runtime we want to get the xxx=yyy.

the new solution: SerdeLambda( see org.ray.util.LambdaUtils). It has the following advantages:

support fo get the lambda' meta-information in runtime.
no need to rewrite the jar. Easy to develop and test.
one solutuion. the code is simpler
fast enough(see LambdaUtilsTest): in mac-air 1.6G I5
- hit the lambda cache, parse the lambda spent 400us+, 80x than remote-lambda
- miss the lambda cache , parse the lambda spent 2600us+, 200x than remote-lambda

AmplabJenkins · 2018-06-12T15:10:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6025/
Test PASSed.

pcmoritz · 2018-06-12T21:27:07Z

java/common/src/main/java/org/ray/util/MethodId.java

+      CACHE = ThreadLocal.withInitial(() -> new WeakHashMap<>());
+
+  /**
+   * format A.B.C.cname


What is the meaning of this comment?

ok, I would clean it.

pcmoritz · 2018-06-12T21:27:26Z

java/common/src/main/java/org/ray/util/MethodId.java

+
+public final class MethodId {
+
+  /* use ThreadLocal to avoid lock. </br>


unmatched
?

ok, I would clean it.

pcmoritz · 2018-06-12T21:29:51Z

java/common/src/main/java/org/ray/util/MethodId.java

+import org.ray.util.logger.RayLog;
+
+
+public final class MethodId {


Can we add a small comment on what this class is doing? I.e. it seems to be similar to our FunctionID in Ray, except it also has a class associated with it? Also, let's document the fields methodDesc and encoding, it's not clear to me at the moment what they are.

ok, I would add the doc

pcmoritz · 2018-06-12T21:31:13Z

java/runtime-common/src/main/java/org/ray/core/LocalFunctionManager.java

+      if (m != null) {
+        return m;
+      }
+      //it is a actor static func


there should be a space after the //

ok, I would clean it.

pcmoritz · 2018-06-12T21:31:28Z

java/runtime-common/src/main/java/org/ray/core/LocalFunctionManager.java

-      }
+  Pair<ClassLoader, RayMethod> getMethod(UniqueID driverId, UniqueID actorId,
+      UniqueID methodId, String className) {
+    //assert the driver's resource is load


space after //

ok, I would clean it.

pcmoritz · 2018-06-12T21:31:46Z

java/runtime-common/src/main/java/org/ray/core/ArgumentsBuilder.java

  public static Pair<Object, Object[]> unwrap(TaskSpec task, Method m, ClassLoader classLoader)
      throws TaskExecutionException {
-    FunctionArg[] fargs = task.args;
+    //the last arg is className


space after //

ok, I would clean it.

pcmoritz · 2018-06-12T21:32:58Z

java/runtime-native/src/main/java/org/ray/spi/impl/JarLoader.java

+  }
+
+  public static void unloadJars(ClassLoader loader) {
+    // TODO:


say what is to be done here (or if nothing, remove the todo)

ok, I would add the doc

pcmoritz · 2018-06-12T21:36:40Z

Thanks, this is really great! Some high level questions:

How do performance numbers you cite compare to the old solution with the binary rewrite? (this solution is clearly superior but it would be good to know the difference)
Are there any changes in semantics for the user? The changes in [RFC] Improve Java API #2172 are orthogonal to this, right?

Also if functions are garbage collected from the WeakDictionary, they will be reloaded later, right? Where are the remote lambdas stored?

with issue ray-project#2245

raulchen · 2018-06-13T03:24:03Z

.gitignore

+*.iml
+java/test
+java/*/target
+java/target/


Don't ignore java/test, it's a valid module.
java/test and java/*/target can be combined as java/**/target

raulchen · 2018-06-13T03:26:31Z

java/common/src/main/java/org/ray/util/LambdaUtils.java

+  }
+
+
+  public static SerializedLambda getSerializedLambda(Serializable lambda) {


Let's also use RayFunc as the parameter type here.

MethodId is not only for RayFunc, it clould used for all lambda call. the check of RayFunc has add in entrance

raulchen · 2018-06-13T03:27:12Z

java/common/src/main/java/org/ray/util/MethodId.java

+   * Note: the lambda instances are dynamically created per call site,
+   * we use WeakHashMap to avoid OOM.
+   */
+  private static final ThreadLocal<WeakHashMap<Class<Serializable>, MethodId>>


same here, RayFunc.
and a lot of other places in the rest of this PR.

MethodId is not only for RayFunc, it clould used for all lambda call. the check of RayFunc has add in entrance

raulchen · 2018-06-13T03:28:48Z

java/common/src/main/java/org/ray/util/MethodId.java

+    this.encoding = encode(className, methodName, methodDesc, isStatic);
+    this.digest = getSha1Hash0();
+  }
+


nit: remove empty line

raulchen · 2018-06-13T03:29:37Z

java/common/src/main/java/org/ray/util/MethodId.java

+      final SerializedLambda lambda = LambdaUtils.getSerializedLambda(serial);
+      Preconditions.checkArgument(lambda.getCapturedArgCount() == 0, "could not transfer a lambda "
+          + "which is closure");
+      final boolean isstatic = lambda.getImplMethodKind() == MethodHandleInfo.REF_invokeStatic;


nit: isStatic

raulchen · 2018-06-13T03:50:22Z

@pcmoritz

The perf of this solution is slightly worse than binary rewrite, but should be totally good enough for prod (see commit message).
If everybody is okay with this PR, we'll go on with option 1.
Yes, If a function's GC-ed, it will be reloaded. (To be more specific, we're not caching the function, we're caching the metadata of the function).

mylinyuzhi · 2018-06-13T04:08:02Z

performance

binary rewrite is fastest. the functioId has computed in rewrite. No time overhead for functioId get.
the serdeLambda need to compute the functioInd when miss cache. it need to parse the lambda and sha1 the MethodId. in my mac air, it need 2000+ ns. if hit the cache, it only need 20+ ns.
(time cost of System.currentTimeMillis is 50ns)
Actually, event if not hit the cache, 2000ns is not a bottleneck.
And, avoid the overhead of data trans#ission(600bytes) in remotelambda.

changes in semantics for the user

the serdeLambda didn't change the semantics for the user. the PR keep the Ray.call semantics. Do not conflict with this #2172

functions gc

the java func would not be gc, it only gc the MethodId. MethodId is a point to the called java function.
if miss the MethodId, the MethodId would be reconstruct.

what is the meaning of 'remote lambdas stored' ? I think, there is no need to store them.

AmplabJenkins · 2018-06-13T05:21:14Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6047/
Test PASSed.

AmplabJenkins · 2018-06-13T08:12:44Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6051/
Test FAILed.

pcmoritz

LGTM, thanks for the changes!

* 'master' of https://github.com/ray-project/ray: (157 commits) Fix build failure while using make -j1. Issue 2257 (ray-project#2279) Cast locator with index type (ray-project#2274) fixing zero length partitions (ray-project#2237) Make actor handles work in Python mode. (ray-project#2283) [xray] Add error table and push error messages to driver through node manager. (ray-project#2256) addressing comments (ray-project#2210) Re-enable some actor tests. (ray-project#2276) Experimental: enable automatic GCS flushing with configurable policy. (ray-project#2266) [xray] Sets good object manager defaults. (ray-project#2255) [tune] Update Trainable doc to expose interface (ray-project#2272) [rllib] Add a simple REST policy server and client example (ray-project#2232) [asv] Pushing to s3 (ray-project#2246) [rllib] Remove need to pass around registry (ray-project#2250) Support multiple availability zones in AWS (fix ray-project#2177) (ray-project#2254) [rllib] Add squash_to_range model option (ray-project#2239) Mitigate randomly building failure: adding gen_local_scheduler_fbs to raylet lib. (ray-project#2271) [rllib] Refactor Multi-GPU for PPO (ray-project#1646) [rllib] Envs for vectorized execution, async execution, and policy serving (ray-project#2170) [Dataframe] Change pandas and ray.dataframe imports (ray-project#1942) [Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) (ray-project#2245) ...

<feature> : serde lambda

4869e97

pcmoritz changed the title ~~<feature> : serde lambda~~ Replace binary rewrite with Remote Lambda Cache (SerdeLambda) Jun 12, 2018

pcmoritz changed the title ~~Replace binary rewrite with Remote Lambda Cache (SerdeLambda)~~ [Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) Jun 12, 2018

pcmoritz self-assigned this Jun 12, 2018

pcmoritz reviewed Jun 12, 2018

View reviewed changes

<feature>:fixed CR

b24129f

with issue ray-project#2245

raulchen requested changes Jun 13, 2018

View reviewed changes

raulchen mentioned this pull request Jun 13, 2018

[RFC] Improve Java API #2172

Closed

<feature>: fixed CR

e61ed28

pcmoritz approved these changes Jun 13, 2018

View reviewed changes

pcmoritz merged commit fa0ade2 into ray-project:master Jun 13, 2018


		public final class MethodId {

		/* use ThreadLocal to avoid lock. </br>

		import org.ray.util.logger.RayLog;


		public final class MethodId {

		}


		public static SerializedLambda getSerializedLambda(Serializable lambda) {

[Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) #2245

[Java] Replace binary rewrite with Remote Lambda Cache (SerdeLambda) #2245

Uh oh!

Conversation

mylinyuzhi commented Jun 12, 2018

What do these changes do?

Uh oh!

AmplabJenkins commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmoritz commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulchen commented Jun 13, 2018

Uh oh!

mylinyuzhi commented Jun 13, 2018

performance

changes in semantics for the user

functions gc

Uh oh!

AmplabJenkins commented Jun 13, 2018

Uh oh!

AmplabJenkins commented Jun 13, 2018

Uh oh!

pcmoritz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!