[Skylark] Use POJOs instead of dynamic proxies. #5666

ttsugriy · 2018-07-25T01:05:43Z

Java uses dynamically generated proxy classes to access annotation properties
and their methods are ~7X slower than plain getters. According to async-profiler
50%+ of convertArgumentList method time is spent in dynamic proxy methods, so
optimizing their performance makes sense.

This also makes the model less anemic, since POJOs can actually provide business
methods.

ttsugriy · 2018-07-25T01:09:26Z

proof of proxy vs getter perf - https://gist.github.com/ttsugriy/c5c7719b9d7bb993d62f445a0be94e5c

ttsugriy · 2018-07-25T15:49:10Z

@laurentlb, @dslomov, would you mind taking a look? On top of having ~25% parse time improvement this change also enables further optimizations - I'll move the getType computation to a ParamDescriptor.of static factory method which should eliminate the next bottleneck - going through loading cache to resolve types, since types have to be resolved for every single method invocation and for each parameter.

c-parsons

Just yesterday I was looking into doing something similar, but did not find that it had a substantial improvement on analysis time of large builds -- did you truly find a 25% improvement for this change? (How large is the build of concern, out of curiosity?)

c-parsons · 2018-07-25T16:14:14Z

src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java

+  }
+
+  /** A value class for storing {@link Param} metadata to avoid using Java proxies. */
+  public static final class ParamDescriptor {


Could you please move this class to the top level? Long class definitions, even if they're trivial, make reading the containing class a bit more daunting.

absolutely, I've used a nested class because MethodDescriptor is nested, but given how big all of these classes are, I agree that separating them makes code more readable :)

c-parsons · 2018-07-25T16:15:15Z

src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java

  /**
-   * A value class to store Methods with their corresponding SkylarkCallable annotations.
-   * This is needed because the annotation is sometimes in a superclass.
+   * A value class to store Methods with their corresponding SkylarkCallable annotations. This is


Update javadoc

Does this now contain all of the information that was before stored in SkylarkCallable? Then we can remove that field

It does, but unfortunately getAnnotation is still used in SkylarkSignatureProcessor :(

c-parsons · 2018-07-25T16:17:55Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java

-        throw new RuntimeException(String.format(
-            "Exception while processing @SkylarkSignature.Param %s, default value %s",
-            param.name(), param.defaultValue()), e);
+      if (defaultValue.isEmpty()) {


Why the change here?

it's so that this method can be used by clients that have ParamDescriptor or Param annotation. It's somewhat unfortunate since having parameters of the same type is a bit confusing, but until all usages of annotations are removed this is the way to avoid duplication :(

Apologies for the merge conflict here with dc6f1cb, (I had it pending before I saw your PR, and it just got its review stamped)
In brief, I found another large source of performance suffering in that we evaluate default values repeatedly throughout function invocations. Spinning up and evaluating a new AST is expensive, so caching the values is best.
The merge shouldn't be too bad.

It does make me wonder, however, if there are confounding factors with your ~50% proxy methods citation. (In truth, I'm more trying to reconcile my previous findings that proxy methods had little to no impact). One thing I did notice is that Param->SkylarkType conversion was expensive, and both my commit and yours resolve that.

Regardless, your change LGTM (after resolving merge conflicts), so this is mostly an aside.

no problem at all, @c-parsons! Thanks for working on these changes! The only explanation I can think of with regards to your proxy overhead observation is that in the code you have used for benchmarking you didn't have a lot of parameters/methods. Since Buck does not have extension support, we end up having extremely complicated macros with hundreds (200+) parameters, so for us it ends up using a lot of cycles :( I'll gladly rebase my changes. Thanks again for working on Skylark perf and your reviews!

btw, @c-parsons, what do you think about moving the parsed defaultValue to the ParamDescriptor? At the moment a lot of time is spent in a LoadingCache, so storing the variable directly next to a parameter can make for a potentially easier to use API and better for performance. If you don't mind, I'll be happy to do this.

ttsugriy

thanks for taking a look, @c-parsons! I have indeed observed a very noticeable (25+%) parse time improvement for our internal android apps and these proxy invocations have totally disappeared in profiler :) If you're curious I can also share async-profiler samples before and after this change :)

ttsugriy · 2018-07-25T16:47:59Z

src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java

-        throw new RuntimeException(String.format(
-            "Exception while processing @SkylarkSignature.Param %s, default value %s",
-            param.name(), param.defaultValue()), e);
+      if (defaultValue.isEmpty()) {


it's so that this method can be used by clients that have ParamDescriptor or Param annotation. It's somewhat unfortunate since having parameters of the same type is a bit confusing, but until all usages of annotations are removed this is the way to avoid duplication :(

laurentlb · 2018-07-25T17:05:52Z

cc @meisterT

ttsugriy · 2018-07-25T23:03:18Z

@c-parsons, I've effectively merged the getType caching from my other PR into this one, so please take a look at this one and I'll close the other one :)

c-parsons

Looks great. Thanks so much for this! I have just two nits to pick, but I'm approving :)

c-parsons · 2018-07-26T15:21:55Z

src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java

+    this.named = named;
+    this.legacyNamed = legacyNamed;
+    this.positional = positional;
+


nit: kill extra newline

c-parsons · 2018-07-26T15:23:00Z

src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java

+ * metadata. This is needed because the annotation is sometimes in a superclass.
+ *
+ * <p>The annotation metadata is duplicated in this class to avoid usage of Java dynamic proxies
+ * which are ~7X slower


nit: period at end of javadoc

Java uses dynamically generated proxy classes to access annotation properties and their methods are ~7X slower than plain getters. According to async-profiler 50%+ of `convertArgumentList` method time is spent in dynamic proxy methods, so optimizing their performance makes sense. This also makes the model less anemic, since POJOs can actually provide business methods.

ttsugriy · 2018-07-26T15:54:04Z

Thanks for the supersonic review, @c-parsons! I've addressed the nits, so please take another look when you have a chance :)

laurentlb · 2018-07-30T17:58:52Z

I made small changes when importing your PR to fix lint issues (copyright header, unused fields, functions with same name should be grouped).

ttsugriy · 2018-07-30T18:56:06Z

Thank you, @laurentlb!

Java uses dynamically generated proxy classes to access annotation properties and their methods are ~7X slower than plain getters. According to async-profiler 50%+ of `convertArgumentList` method time is spent in dynamic proxy methods, so optimizing their performance makes sense. This also makes the model less anemic, since POJOs can actually provide business methods. Closes bazelbuild#5666. PiperOrigin-RevId: 206608812

googlebot added the cla: yes label Jul 25, 2018

laurentlb requested a review from c-parsons July 25, 2018 15:51

c-parsons reviewed Jul 25, 2018

View reviewed changes

ttsugriy force-pushed the no-callable-proxy branch from 5a5be69 to c8afda8 Compare July 25, 2018 16:57

ttsugriy commented Jul 25, 2018

View reviewed changes

ttsugriy force-pushed the no-callable-proxy branch from c8afda8 to 83b3ba3 Compare July 25, 2018 17:00

dkelmer assigned c-parsons Jul 25, 2018

ttsugriy force-pushed the no-callable-proxy branch 2 times, most recently from 8c18f12 to f7a902b Compare July 25, 2018 23:02

ttsugriy mentioned this pull request Jul 25, 2018

Store SkylarkType in a ParamDescriptor. #5670

Closed

ttsugriy force-pushed the no-callable-proxy branch from f7a902b to 8c7dcdb Compare July 25, 2018 23:16

c-parsons approved these changes Jul 26, 2018

View reviewed changes

ttsugriy force-pushed the no-callable-proxy branch from 8c7dcdb to 4e1df17 Compare July 26, 2018 15:52

c-parsons approved these changes Jul 26, 2018

View reviewed changes

ttsugriy mentioned this pull request Jul 27, 2018

[Skylark] Size argument list builder to avoid allocations. #5694

Closed

bazel-io closed this in 7dbc5e0 Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Skylark] Use POJOs instead of dynamic proxies. #5666

[Skylark] Use POJOs instead of dynamic proxies. #5666

ttsugriy commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

c-parsons left a comment

c-parsons Jul 25, 2018

ttsugriy Jul 25, 2018

c-parsons Jul 25, 2018

ttsugriy Jul 25, 2018

c-parsons Jul 25, 2018

ttsugriy Jul 25, 2018

c-parsons Jul 25, 2018

ttsugriy Jul 25, 2018

ttsugriy Jul 25, 2018 •

edited

Loading

ttsugriy left a comment

ttsugriy Jul 25, 2018

laurentlb commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

c-parsons left a comment

c-parsons Jul 26, 2018

c-parsons Jul 26, 2018

ttsugriy commented Jul 26, 2018

laurentlb commented Jul 30, 2018

ttsugriy commented Jul 30, 2018

[Skylark] Use POJOs instead of dynamic proxies. #5666

[Skylark] Use POJOs instead of dynamic proxies. #5666

Conversation

ttsugriy commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

c-parsons left a 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

ttsugriy Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

ttsugriy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentlb commented Jul 25, 2018

ttsugriy commented Jul 25, 2018

c-parsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttsugriy commented Jul 26, 2018

laurentlb commented Jul 30, 2018

ttsugriy commented Jul 30, 2018

ttsugriy Jul 25, 2018 •

edited

Loading