I am definitely for this kind of optimization, since GQuery is
effectively nothing but chained calls. However, I think the issue is
more complicated than just rewriting.
First, whether or not the optimization is an improvement depends on
subsequently whether or not the methods can be inlined. Thus, if
setA()/setB()/setC() can't be inlined, then you have increased code
size. This can occur because the methods might be polymorphic, they
might be too big, they might accidently evaluate the parameters in the
wrong order, etc.
Secondly, it doesn't appear to be as simple as checking for 'return
this'. Consider
public Builder setA(int val) {
if(isInvalid(val)) {
return this;
}
else {
return this;
}
}
vs
public Builder setA(int val) {
if(isInvalid(val)) {
return new Builder(this);
}
else {
return this;
}
}
vs
public Builder setA(int val) {
return UtilityClass.someMethod(this, val); // actually used by GQuery
}
I actually do think your proposal will work, although I'd feel much
better if it were more general purpose instead of tied so specifically
to a recognizable pattern, but I think the actual implementation may
be more tricky if you want to make it robust.
-Ray
On Tue, Jun 16, 2009 at 9:06 AM, Freeland Abbott<[email protected]> wrote:
> As I said, I hadn't looked at all into how feasible this was... but I was
> hoping to do it by jiggering the Java AST, really, and not the JS one. That
> is, I was looking to rewrite the Java from a chained
> builder.setA().setB().setC() to multi-statement
> builder.setA();builder.setB();builder.setC(), effectively unwinding any call
> on the return value of a method returning "this" as a special-case pattern,
> and let the Java-to-JavaScript compiler run from that.
>
>
>
> On Tue, Jun 16, 2009 at 11:59 AM, Ray Cromwell <[email protected]>
> wrote:
>>
>> Even before the inliner gets it, a JSO chained expression like
>> a().b().c() is going to look like c(b(a())) so the inliner can only
>> inline the first call.
>>
>> Automatically introducing temporaries would seem a lot harder. You'd
>> have to teach the compiler that 'this' is effectively final, and
>> therefore any method returning this, effectively always returns the
>> same value, regardless of side effects. Then, you'd have to introduce
>> the idea that the compiler can introduce temporaries (speculatively),
>> and later use common subexpression elimination to detect multiple
>> identical ones and hoist them out.
>>
>> e.g.
>>
>> c(b(a()))
>>
>> becomes
>>
>> var t1, t2, t3;
>> t1 = c(t2 = b(t3 = a()))
>>
>> then, recognizing the that t1=t2=t3=builder=a() with copy propagation
>> t1 = builder
>> a(t1)
>> b(t1)
>> c(t1)
>>
>> and now they can be inlined.
>>
>> This just seems hard to me, since introducing temporaries is
>> speculative and you'd have introduce another pass to remove them in
>> the cases where they provided no benefit. However, this could end up
>> leading to infinite loops if you're not careful.
>>
>> -Ray
>>
>>
>>
>> On Mon, Jun 15, 2009 at 6:11 PM, Brian Stoler<[email protected]> wrote:
>> >
>> > Hi gwtc,
>> >
>> > I was playing with using super-source to emulate some existing code
>> > that uses a builder pattern and make it efficient in GWT as a
>> > JavaScriptObject. I was hoping that the I could make the Builder just
>> > compile away, but that doesn't seem to be happening. I started
>> > debugging in JsInliner but started to get a bit lost. This is using
>> > svn r5557 on trunk FYI (very recent).
>> >
>> > What I am seeing is that if you use a chaining call pattern, the
>> > inliner doesn't realize some of the optimization it can do.
>> >
>> > Example usage code:
>> >
>> > void example() {
>> > MyData myData = MyData.Builder.create()
>> > .setBar("a")
>> > .setFoo("b").build();
>> >
>> > Window.alert(myData.getBar());
>> > Window.alert(myData.getFoo());
>> > }
>> >
>> >
>> > Output I get (extraneous bits removed, PRETTY mode):
>> >
>> > function init(){
>> > var myData;
>> > myData = $setFoo($setBar(new Object(), 'a'), 'b');
>> > $wnd.alert(myData.bar);
>> > $wnd.alert(myData.foo);
>> > }
>> >
>> > function $setBar(this$static, s){
>> > this$static.bar = s;
>> > return this$static;
>> > }
>> >
>> > function $setFoo(this$static, s){
>> > this$static.foo = s;
>> > return this$static;
>> > }
>> >
>> >
>> > However, with this similar code:
>> >
>> > void example2() {
>> > MyData.Builder builder = MyData.Builder.create();
>> > builder.setBar("a");
>> > builder.setFoo("b");
>> >
>> > MyData myData = builder.build();
>> >
>> > Window.alert(myData.getBar());
>> > Window.alert(myData.getFoo());
>> > }
>> >
>> >
>> > The code compiles better (no setters):
>> >
>> > builder = new Object();
>> > builder.bar = 'a';
>> > builder.foo = 'b';
>> > myData = builder;
>> > $wnd.alert(myData.bar);
>> > $wnd.alert(myData.foo);
>> >
>> >
>> > The biggest issue seemed to be that the inliner didn't introduce a
>> > local variable for "new Object()", and because of that, it correctly
>> > concluded it couldn't keep copying that expression around. (I traced
>> > through it having issues due to "new" having side effects.) Even if I
>> > manually create a variable for the Builder at the outset, it still
>> > doesn't help. (FYI, I am using new Object() instead of {} to work
>> > around
>> > http://code.google.com/p/google-web-toolkit/issues/detail?id=3568)
>> >
>> > I was going to try to dig some more in to the inliner, but any ideas
>> > how to improve this?
>> >
>> > Will a later pass remove useless variable aliasing? (Seems like not
>> > from my other example?) If so, I was thinking it might work to have
>> > the inliner introduce a local all the time when it is trying to inline
>> > a non-void function. Smarter methods are of course possible, but I
>> > thought that might be worth a shot.
>> >
>> > Any other ideas welcome. MyData class attached below. I tried a
>> > variety of small tweaks to the MyData class and nothing made a
>> > difference.
>> >
>> > FYI, my eventual goal is:
>> >
>> > var myData = {bar:'a',foo:'b'};
>> >
>> > But that seems like an orthogonal optimization (and perhaps
>> > generically useful).
>> >
>> > Thanks for your interest, if you read this far,
>> > -brian
>> >
>> > PS: I am open to any changes to the MyData code below that a) preserve
>> > the JSO-ness and b) leave the external API the same.
>> >
>> > public static interface MyData {
>> > String getFoo();
>> > String getBar();
>> >
>> > public static final class Builder extends JavaScriptObject
>> > implements MyData {
>> > protected Builder() {}
>> >
>> > public static native Builder create() /*-{
>> > return new Object();
>> > }-*/;
>> >
>> > public native String getFoo() /*-{
>> > return this.foo;
>> > }-*/;
>> >
>> > public native String getBar() /*-{
>> > return this.bar;
>> > }-*/;
>> >
>> > public Builder setFoo(String s) {
>> > setFooImpl(s);
>> > return this;
>> > }
>> >
>> > private native void setFooImpl(String s) /*-{
>> > this.foo = s;
>> > }-*/;
>> >
>> > public Builder setBar(String s) {
>> > setBarImpl(s);
>> > return this;
>> > }
>> >
>> > private native void setBarImpl(String s) /*-{
>> > this.bar = s;
>> > }-*/;
>> >
>> > public MyData build() {
>> > return this;
>> > }
>> > }
>> > }
>> >
>> > >
>> >
>>
>>
>
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
