eliminate dynamicCast for generics, or all?

[Ray Cromwell]

Here's the scoop. I changed a field in Chronoscope from RangeAxis[] to List<RangeAxis> and suffered ~50% performance regression. An Axis is a class responsible for mapping values in the range [low, high] to [0,1], among other things, so it is a hot class/method in Chronoscope. After profiling, I noticed the method was no longer being inlined, and a huge number of calls to dynamicCast/canCastUnsafe appeared. This brings to mind two optimizations:

1) if a method's return type is covariant in a generic type, do not insert a cast or cast check. Assume that the method *will* return objects of this type at the callsite.

2) eliminate dynamicCast/canCastUnsafe altogether. These methods only exist to throw class cast exceptions. I can't believe that there's any code that actually expects these exceptions and depends on them being thrown except perhaps unit tests. Anyone running unit tests or UI tests in hosted mode will get these exceptions during development, so it seems the only use case might be web-mode unit tests.

But for production deployed code, dynamicCast/canCaseUnsafe/setCheck/et al seem to just be bloat and a performance hit to boot. Can we get a compile mode that removes them? Maybe an -XX method, or some other mechanism which says "I want potentially unsafe, but super fast code"?

-Ray

[Ray Cromwell]

BTW, I don't mean to imply that the change from native array to List is the whole of the 50% regression, I count that the dynamicCast/canCaseUnsafe is adding 10% overhead. The rest comes from some other issues, like get() not getting inlined, and some other recent changes that have nothing to do with this.

-Ray

[Scott Blum]

I think it's a worthwhile idea, we've talked before about some "make my code fast but unsafe" options.

[Ray Cromwell]

I have another idea I'd like to run past you. I've noticed that alot of times, method inlining is hampered by the way methods are "staticified". Let's say you have a method:

public class Foo {

  int x;

  public int add(int y) {

     return y + x;

  }

}

this will end up as Javascript

function $add(this$static, y) {

  return y + this$static.x;

}

which will cause the JS Inliner to avoid inlining due to the parameters being evaluated in a different order in the body of the function. That's because the this$static reference is always passed as the first argument. However, if the staticifier was changed to inspect the body of the the function, and insert the this$static reference in the parameter list, according to the order in which references to it were encountered, it might allow the JS Inliner to work better, in the absense of fixing JS Inliner to deal with out-of-order evaluation.

-Ray

[Andrés Testi]

Ray:

What happends if you extends ArrayList to avoid generics?:

class RangeAxisArrayList extends ArrayList<RangeAxis>{}

- Andrés

[Bruce Johnson]

Does anybody actually have a patch that does any of this? I'd love to see numbers on flags like:

-Xopt:skipGenericCasts
-Xopt:skipArrayElemTypechecks
-Xopt:eagerClinits
-Xopt:obfuscatedClassLiterals

I think there's a bunch of pretty low-hanging fruit here.

[Ray Cromwell]

I just looked at doing this. The first target seems to be in GenerateJavaAST:

private JExpression maybeCast(JType expected, JExpression expression) {
if (expected != expression.getType()) {
// Must be a generic; insert a cast operation.
JReferenceType toType = (JReferenceType) expected;
return new JCastOperation(program, expression.getSourceInfo(), toType,
expression);
} else {
return expression;
}
}

If I add a check for an option, and make this routine a no-op, is it
going to work, or break something? Alternatively, I can replace calls
to canCastUnsafe/dynamicCast with no-ops further down in the
processing and get rid of all casts.

The setCheck code appears to be in ArrayNormalizer:

/*
* See if we need to do a checked store. Primitives and (effectively)
* final are statically correct.
*/
if (elementType instanceof JReferenceType) {
if (!((JReferenceType) elementType).isFinal()
|| elementType != x.getRhs().getType()) {
// replace this assignment with a call to setCheck()
JMethodCall call = new JMethodCall(program, x.getSourceInfo(),
null, setCheckMethod);
call.getArgs().add(arrayRef.getInstance());
call.getArgs().add(arrayRef.getIndexExpr());
call.getArgs().add(x.getRhs());
ctx.replaceMe(call);
}
}

Seems easy to preference the outer-conditional here with
"arrayTypeCheckOption.isSkipTypeCheckEnabled()"


The latter two, I'm not exactly sure of the semantics you're asking
for. Seems like eagerly evaluating clinits could be complicated,
especially if someone is counting on their deferment and
order-of-evaluation properties. I think you'd need a control flow
graph to do it properly.

What does obfuscated class literals mean? Do you mean obfuscating the
packagename/classname/supername passed into the createForClass method?

I have noticed a huge number of getClass() methods in my output
Javascript, the vast majority of them unused. They can't be pruned it
appears because of invocations on Object.getClass(), which rescues all
of them. There are only 4 calls to Object.getClass() in my output it
appears. The first is in the default toString() method. The second is
in RuntimeException.getName(). The third is safety checks in arraycopy
(could also eliminate in an optimization mode) I could do without
either of these in OBF mode. The fourth is in serialization code.
Reducing these is something to think about.

-Ray

[Ray Cromwell]

BTW,
What is the best way to access command line options from within
visitor classes? JJSCompiler has them, but it seems you cannot reach
the JJSOptions via a JProgram reference, which means you'd have to
patch JJSCompiler everytime you add an option to pass the options to
the visitors as parameters or fields.

-Ray

[Scott Blum]

On Thu, Jan 29, 2009 at 3:41 PM, Ray Cromwell <cromw...@gmail.com> wrote:

If I add a check for an option, and make this routine a no-op, is it
going to work, or break something? Alternatively, I can replace calls
to canCastUnsafe/dynamicCast with no-ops further down in the
processing and get rid of all casts.

This is going to break stuff because the return type of the called method will be seen to be Object without the cast, which will destroy TypeTightener's ability to optimize, and generally the AST won't be valid without it.  What you would need to do is modify the JMethodCall being produced to override the return type of the target JMethod given your better generic information.

Scott

[Ray Cromwell]

Scott,
Do you mean the JMethodCall produced by
CastNormalizer.ReplaceTypeChecksVisitor? I looked at doing this, but
it seems by the time you get to this stage, you know longer can tell
the difference between a cast inserted automatically because of
generics, or a cast inserted by the programmer (from JDT
CastExpression). I suppose an extra field could be added to
JCastOperation to tell if it came from an automatically generated
cast, but perhaps this is not what you were thinking of?


-Ray

[Bruce Johnson]

I'm long since unfamiliar with the specifics of that code, but it seems like designating a JCastOperation as "generated by the compiler" would be useful, then during actual code generation, you'd just avoid emitting the runtime cast code.

[Scott Blum]

I meant attacking the problem one level higher... instead of generating the synthetic cast, you generate a different JMethodCall or JFieldRef with the type overridden.

But what Bruce suggests might be even better.

[Ray Cromwell]

I think it would be useful to, but on second look, there appear to be
a number of different contexts in which JCastOperation is generated by
the compiler, rather than present in the original source:

1) when converting JDT expressions involving generics
2) when type-tightening
3) when simplifying expressions
4) in MethodInliner when cloning parameters
5) when autoboxing (primitive casts)
6) when normalizing JSOs

So it seems there are three classes of casts. Those authored by the
user, those generated for generics due to type-erasure, and those
generated during optimization.

Scott,
I gotcha now. I see obvious places to do this in
processExpression(MessageSend x) and JExpression
processExpression(FieldReference x), but due to my lack of JDT
knowledge, I don't quite understand what's going on in the
processExpression(QualifiedNameReference x) and
processExpression(SingleNameReference x) routines, so I'm not sure if
anything has to be done in those. If follow Bruce's suggestion and
simply tag JCastOperations based on their reason for insertion, it
seems conceptually simpler to remove those in a latter compiler pass,
and avoid risking breaking GenerateJavaAST, which honestly is code in
which I don't understand the specifics of a lot of what's going on.

-Ray

[Scott Blum]

Nobody understands GenerateJavaAST.  We just hack on it until it does the right thing. :)

[David]

Sounds like a maintenance problem! Time to refactor ?

[Ray Cromwell]

I think it's more like no one's brain is large enough to keep full
cognition of 3 complete AST APIs concurrently, and the GWT team lacks
resources to have a guy who's sole purpose is to own JDT->GwtAST.
Looking at the comments, the JDT AST isn't exactly a pleasure to work
with either. There's comments in the source for non-obvious stuff, but
it still requires indepth understanding of JDT, and I don't know if I
have enough brain cells to spare. :)

Scott's brain on the other hand is a different story. You'll note from
photos his skull is about 20% larger than the average persons. :)

-Ray

[Scott Blum]

On Fri, Jan 30, 2009 at 2:58 AM, Ray Cromwell <cromw...@gmail.com> wrote:

I think it's more like no one's brain is large enough to keep full
cognition of 3 complete AST APIs concurrently, and the GWT team lacks
resources to have a guy who's sole purpose is to own JDT->GwtAST.
Looking at the comments, the JDT AST isn't exactly a pleasure to work
with either. There's comments in the source for non-obvious stuff, but
it still requires indepth understanding of JDT, and I don't know if I
have enough brain cells to spare. :)

Scott's brain on the other hand is a different story. You'll note from
photos his skull is about 20% larger than the average persons. :)

LMAO.  Ray's close to the truth on JDT.  The problem is that JDT is inconsistent, unintuitive, and not documented.  So GenerateJavaAST is really the evolution of us making assumptions about what JDT is doing, finding out where we were wrong (usually resulting in an InternalCompilerException), and fixing / working around it.  I would say 60-75% of all InternalCompilerException bugs we've ever fixed stem from some misunderstanding of JDT implementation details.

One nasty example that springs to mind: it turns out JDT won't fully resolve certain AST constructs unless you actually turn on bytecode generation.  Another one: if JDT determines an anonymous local class is statically unreachable, it will leave that class in an invalid state.

So the real value of GenerateJavaAST is the fact that it's been tested over time on millions of lines of code, and most of the kinks have now been ironed out.  Of course every time we upgrade JDT we risk introducing new problems. :)

Scott

[Lex Spoon]

On Thu, Jan 29, 2009 at 3:41 PM, Ray Cromwell <cromw...@gmail.com> wrote:

> I just looked at doing this. The first target seems to be in GenerateJavaAST:

I see two general strategies that would get the info on casts:

1. Modify CastNormalizer to simply removes all casts, and see what the
size difference is. This behavior would ultimately be settable by a
flag, assuming it actually gets a significant size improvement.

2. Have a flag in JCast named "alwaysSucceeds". Update CloneVisitor
accordingly. Update GenerateJavaAST to turn this on for generics
casts; it could be inserted for others as well. Update the cast
normalizers to check this flag. This is a setting that could be
enabled by default.

#2 is unsound as described, but is probably sound enough. :) I'd
guess it can only go wrong in places where an IDE would at least give
a warning, e.g. casting to a type parameter. We could iterate to a
better precise definition over time.

Regarding flags, giving each visitor a reference to the entire flag
set would add a lot to the dependencies of each visitor. If instead
they have a set of individual parameters, and JJS sets those
parameters, then only JJS has the heavy dependency. Fewer
dependencies is good. In theory we could unit test visitors in
isolation; the lighter the dependencies, the more plausible this is.

Besides, for doing experiments, it is just as well not to create a
flag. Simply comment in or out the relevant code and rerun the
compile.

Lex

[Ray Cromwell]

On Fri, Jan 30, 2009 at 2:07 PM, Lex Spoon <sp...@google.com> wrote:
>
> 1. Modify CastNormalizer to simply removes all casts, and see what the
> size difference is. This behavior would ultimately be settable by a
> flag, assuming it actually gets a significant size improvement.

This would be my preference actually, and not just for size, but for
performance. When I profile in Firefox, I see that
dynamicCast/canCastUnsafe have a huge number of invocations (easily in
the top 10 hotspots), as well as chewing up about 5-10% of total CPU.
Granted, my application is more intensive than most, but would
definitely be a welcome improvement. I can't see many apps deployed in
web-mode actually caring about accurate class cast exceptions, and
hopefully, most CCEs would be caught much easier in the development
process, in hosted mode, or during unit tests.

-Ray

[Lex Spoon]

That's higher than I realized. Certainly high enough to make a strong
argument for being a touch less safe to gain the speed.

Note, though, that we still don't know how many of those could be
removed if we had a "known to succeed" flag in JCast. It might be
that most of them are due to generics rather than to explicit casts
the user put in. In that case, the overhead might mostly go away
while giving up much less safety.

Lex

[Mark Renouf]

Got bit by this the other day. This is my current workaround for
Comparable, since without it, a dynamicCast is called for each test
(like in a map, using natural odering where a K must be cast to a
Comparable<K>). I use it in tight loops where the dynamicCast overhead
would kill performance (+20% overhead). I hope there is better support
or an official override mechanism on it's way.

package com.google.gwt.util.client;

/**
* Wraps an object and exposes a compareTo method which is executed
without
* runtime type safety checking. This avoids a huge performance hit
in
* certain cases. There is zero type safety, and this runs
significantly slower
* in hosted mode. Use with caution!
*/
public class NativeComparable<K> implements Comparable<K> {
@SuppressWarnings("unused")
private Object a;

public NativeComparable(Object obj) {
this.a = obj;
}
public native int compareTo(K b) /*-{
return
this.@com.google.gwt.util.client.NativeComparable::a.@java.lang.Comparable::compareTo
(Ljava/lang/Object;)(b);
}-*/;
}

On Feb 2, 1:14 pm, Lex Spoon <sp...@google.com> wrote:

[Miroslav Pokorny]

@mark

Your trick as mentioned (using jsni to avoid casting ) was/is used by
the generated rpc serializers to avoid / skip casts ...

Mp