My favorites | Sign in
GWT
Project Home Downloads Wiki Issues
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 4301: issue with toString() and JSNI
27 people starred this issue and may be notified of changes. Back to list
 
Reported by sanjiv.j...@gmail.com, Dec 3, 2009
Found in GWT Release : 2.0 RC2

Consider the following code

    public void onModuleLoad() {
        Map map = new HashMap();
        map.put("image", "yahoo_mobile.png");

        String urlToString = map.get("image").toString();

        String url = "yahoo_mobile.png";

        analyze(urlToString, url);

    }

    private static native void analyze(String toString, String str)/*-{
        debugger;
        alert(typeof toString);
        alert(typeof str);
    }-*/;


The first variable urlToString is essentially a the value obtained by calling toString() on an 
underlying String object but at the Object level. The second variable "url" is a regular String. 
When passing both these Strings to a JSNI method, the String obtained by the call toString() is an 
array of characters and the variable typeof is "object", while the other String "url" is a pain old 
Javascript string with typeof "string".  Run the above code in web mode with Firebug enabled and 
the breakpoint will be hit and you can compare the two types.  See attached screenshot.

The issue is that the value obtained via toString is not a real JavaScript String and when this is 
passed along to some other Javascript functions / third party JS, it fails as it gets treated as an 
array of characters. The side effect depends on what the third party JS is doing with it, but it 
definitely does not get the String type it was expecting.

The issue exists in FF 3.5.5 and IE 7, but the code works in dev mode as well as Chrome web 
mode.


toString-issue.png
185 KB   View   Download
Dec 3, 2009
Project Member #1 t.broyer
Here's what the generated code for gecko1_8 looks like the following code (see below; 
I've only kept what's needed to understand).

What's remarkable is that in the end it's equivalent to:
   String.prototype.my_very_own_toString = function() { return this; }
   map = {};
   map[':image'] = 'some string';
   my_string = map[':image'].my_very_own_toString();
   my_other_string = 'some other string';
   alert(typeof my_string); // object
   alert(typeof my_other_string); // string
I've added two more alerts:
   alert(typeof map[':image']); // string
   alert(typeof my_other_string.my_very_own_toString()); // object

What this shows is that it's the same "bug" in *both* IE (tested in IE8, in both 
Quirks and IE8 modes) and Firefox (tested in 3.6b4): they seem to use some kind of 
"boxing" which can turn a String ("primitive type") into a String "object". The 
'this' in my_very_own_toString() is of type 'object', not 'string' (I've added an 
alert(typeof this) to confirm).

I've reduced the test case to:
   String.prototype.my_own_toString = function() { return this; }
   var foo = "foo";
   alert(typeof foo); // string
   alert(typeof foo.my_own_toString()); // object
   alert(foo == foo.my_own_toString()); // true
   alert(foo === foo.my_own_toString()); // false

The same code in Chrome (tested in 4.x, DevChannel) alerts string,string,true,true

Just tested in Opera 10.10 and Safari 4.0.4 (Windows), same "bug" 
(string,object,true,false), which makes me wonder if it's really a bug in all 4 JS 
engines, or a bug in V8 (the engine used by Chrome).

======================

function toString__devirtual$(this$static){
  return this$static.typeMarker$ == nullMethod || this$static.typeId$ == 2?
this$static.toString$():this$static.toString?
this$static.toString():'[JavaScriptObject]';
}

function init(){
  var map, urlToString;
  !!$stats && $stats({moduleName:$moduleName, sessionId:$sessionId, 
subSystem:'startup', evtGroup:'moduleStartup', millis:(new Date).getTime(), 
type:'onModuleLoadStart', className:'com.atolcd.gwt.client.TestProject'});
  map = $HashMap(new HashMap);
  $putStringValue(map, 'image', 'yahoo_mobile.png');
  urlToString = toString__devirtual$(map.stringMap[':image']);
  analyze(urlToString, 'yahoo_mobile.png');
}

function toString_6(){
  return this;
}

_ = String.prototype;
_.equals$ = equals_1;
_.getClass$ = getClass_19;
_.hashCode$ = hashCode_2;
_.toString$ = toString_6;
_.typeId$ = 2;

function $putStringValue(this$static, key, value){
  var result, stringMap = this$static.stringMap;
  key = ':' + key;
  key in stringMap?(result = stringMap[key]):++this$static.size;
  stringMap[key] = value;
  return result;
}

function $HashMap(this$static){
  this$static.hashCodeMap = [];
  this$static.stringMap = {};
  this$static.nullSlotLive = false;
  this$static.nullSlot = null;
  this$static.size = 0;
  return this$static;
}

Dec 3, 2009
Project Member #2 t.broyer
I haven't tested a patched GWT, but at least the following JS code works (tested in 
FF3.6b4 and IE8). The only difference is that my_own_toString() returns 
'String(this)' instead of 'this'.

   String.prototype.my_own_toString = function() { return String(this); }
   var foo = "foo";
   alert(typeof foo); // string
   alert(typeof foo.my_own_toString()); // object
   alert(foo == foo.my_own_toString()); // true
   alert(foo === foo.my_own_toString()); // false

Dec 3, 2009
#3 sanjiv.j...@gmail.com
Thanks for the detailed analysis Thomas. 

I should have probably mentioned in the issue that changing 

String urlToString = map.get("image").toString();
to 
String urlToString = (String)map.get("image");

works.
Dec 3, 2009
Project Member #4 t.broyer
According to this test, typeof Object("some string") === "object", so it seems all browsers/JS engine except Chrome/V8 
simply treat the 'this' as an Object:
https://code.google.com/p/sputniktests/source/browse/trunk/tests/Conformance/09_Type_Conversion/9.9_ToObject/S9.9_A5.js

I'm now searching for whether this is the correct behaviour; but anyway, it means there's a bug in GWT for not coping 
for the differences between browsers.
Dec 3, 2009
Project Member #5 t.broyer
IIUC sections 11.2.1 and 11.2.3 from ECMAScript 3 (ECMA-262), Chrome/V8 is wrong.
Unfortunately, it doesn't seem to be tested in the Sputnik tests (which AFAIK are
used to ensure V8 complies to ECMAScript 3):
https://code.google.com/p/sputniktests/source/browse/trunk/tests/Conformance/11_Expressions/11.2_Left_Hand_Side_Expressions/11.2.1_Property_Accessors/S11.2.1_A3_T3.js
Dec 4, 2009
Project Member #6 t.broyer
I've filed a bug at V8: https://code.google.com/p/v8/issues/detail?id=538

But given that it looks like GWT is returning a String object, just like 'new 
String("foo")', it might be that your JSNI code or third-party lib (or the browser?) 
has a bug too in not being able to deal with it.

I mean, even if GWT's String::toString returned a String value, there would still be 
cases where your get a String object (e.g. coming from JSNI) and you have to convert 
it to a String value (see r7043 and r7063 as an example, in this case, the bug is in 
FF).

As a side note, am I right assuming this is related to  issue 4283  ?
Dec 4, 2009
#7 sanjiv.j...@gmail.com
 issue 4283  does seem related. 

Can't GWT always pass a string value across the Java -> JSNI boundary for String type?
Dec 5, 2009
#8 charles....@gmail.com
@t.broyer I think you'll probably find that many JS libraries would be surprised by a
String Object and not treat it correctly.  After all, as you've demonstrated, the
most common way of detecting an object's type (the typeof operator) fails for this
case.  Anyone writing a method that takes either an Object or String would be bitten
by this.

However the point is somewhat moot, because what you should really do is get all
those methods off the String prototype and call them on some helper object.  Calling
any method installed to the String prototype counts as one GC "touch" in IE6, in
other words, it contributes to the frequency of garbage collection runs to the same
extent as allocating a new String.  

I didn't realize the GWT team was not aware of this consequence.  We know of several
other obscure GC consequences like this - contact me if you want a run-down.
Dec 15, 2009
#9 fabbott@google.com
(No comment was entered for this change.)
Owner: sp...@google.com
Labels: Category-Compiler
Feb 24, 2010
Project Member #10 scottb@google.com
1) First off, let me say that we know all about the differences between the String 
Object and string primitive in JavaScript.  I consider this one of the nightmares of 
JS programming, and I have no idea how in the world JS developers deal with the 
bizarre autoboxing behavior.  What we have chosen to do in GWT is to simply not 
enforce or care whether the underlying value is a String or string.  Compiled GWT code 
should work perfectly fine in either case.

The problem comes in when you pass these values into JavaScript code that really wants 
it to be a primitive.  In our own JSNI code, in the rare case where it matters we 
simply force it to the primitive, as per here:

https://code.google.com/p/google-web-
toolkit/source/browse/trunk/user/src/com/google/gwt/json/client/JSONObject.java#240

2) In principle, I can't see any problem with modifying String.toString() to return 
String(this) to force it to primitive.  However...

3) Charles brings up a good point in comment #8 about temporary autoboxes contributing 
to GC thrash.  That seems really unfortunate, and in general I don't know how we can 
avoid it.  Wouldn't you hit this problem *any* time you call a method on any string 
(like length(), indexOf(), etc)?  If Charles is right, maybe it's better to prefer 
letting primitives become objects to avoid GC thrash, since once something's already 
being passed around as a wrapper, it shouldn't need to keep rewrapping it.

Cc: sco...@google.com
Feb 24, 2010
#11 charles....@gmail.com
Neither the GC thrash nor the other problems of autoboxing occur if you simply don't
install methods on JavaScript native object prototypes.  So the JS programmers of the
world deal with this mostly by just not installing such methods (with pure JS
libraries it's bad practice from an interoperability perspective) or, for the few
methods where it's worthwhile, being careful to always return the primitive type.

So 99% of ordinary JS developers don't even know about the Object version of
primitives because the libraries they use never produce them.  It would be great if
GWT behaved the same, as Sanjiv said, always passing the primitive type across the
Java->JSNI boundary.

As far as the GC thrash aspect, Any chance of going from:

   String.prototype.equals = function (otherObject) { ... }

.. to ..

   window.gwt.equals = function (obj1, obj2) { ... }

It might be a big upheaval, but:

1. you can stop worrying primitive vs Object type entirely

2. it's going to be a *lot* faster in IE6 (the larger the application, the larger the
difference) and probably faster elsewhere (haven't measured conclusively).  Yes, as
shown above, there's a penalty for "window.gwt" being global scope, but this is
avoidable.

3. you won't have interoperability problems with JavaScript libraries
Feb 25, 2010
Project Member #12 scottb@google.com
It sounds like you're telling me that the String built-in methods (length, etc) don't 
have this behavior, only methods that the user installs.

Unfortunately, we really do need to do true polymorphic dispatch into Strings 
sometimes.  If at some point in your code you have a plain Object.equals() call, we 
have to have a receiver method on the String prototype to implement it.  The 
alternative would be any time we see Object.equals(), we'd have to instead first do a 
runtime check to see whether it's a String or not, then conditionally do normal 
polymorphic vs. special dispatch.

Looping in a couple of folks who care a lot about speed and memory usage.
Cc: j...@google.com knor...@google.com jaime...@google.com cromwell...@google.com
Feb 25, 2010
Project Member #13 t.broyer
Isn't a runtime check what's already done today (kind of)?
(see code in #2: toString__devirtual$ checks for this$static.typeId$ == 2, and a few 
lines below, we see that typeId$==2 identifies a String; well, this is for toString, I 
don't know for equals, getClass and hashCode, as I don't know where this 
toString__devirtual$ code comes from exactly)
Feb 25, 2010
Project Member #14 scottb@google.com
That might be related to SingleJsoImpl... adding Bob.
Cc: b...@google.com
Feb 25, 2010
#15 charles....@gmail.com
To clarify, yes, the built-in methods on the String prototype (eg indexOf) do not
have the autoboxing problem / GC thrash problem. 

Eg we implemented a "startsWith" method like this:

isc.startsWith = function (string, substring) {
   return (string.lastIndexOf(substring, 0) == 0);
}

.. and verified that this causes zero GC touches in IE6 (we had to build a whole
toolset around detecting GC touches).

And yes, if you move equals() so that it's always static instead of installed on
prototypes, it would necessarily involve a big switch/if-then block for determining
the type of the source and target objects.  If there is significant overhead in this,
in many cases the compiler would know the types of the arguments and, as an
optimization, could call a variant that assumes certain types, eg
stringEqualsString(), objectEqualsString(), stringEqualsObject().  You could even
leverage Java generics such that a HashMap<String, Object> would use
stringEqualsString() when comparing keys instead of the more expensive generic equals().

However, just to refocus things, the possible optimizations here can be separated
from the original reason for filing the issue, that is, String Objects being passed
to JSNI code.  Just ensuring that a String Object is never passed into a JSNI block
would be enough to solve this issue.
Feb 25, 2010
Project Member #16 scottb@google.com
Well, I don't think we can ensure that in the general case.  After all, some JSNI 
method might pass us a String Object.  But we could certainly fix String.toString().
Feb 25, 2010
#17 charles....@gmail.com
That would definitely be enough for SmartGWT - we don't have APIs that produce String
Objects - and I would guess it would be enough for most other JavaScript libraries or
code samples that people might use via JSNI.
Jun 13, 2011
#18 dschar...@gmail.com
This bug can be triggered without using any custom JSNI code:

First construct a class whose toString() method returns a JavaScript String object when compiled by GWT.
class StringObject {
	
	private final String string;
	
	public StringObject() {
		// Prevent optimizations by conditioning on Math.random() < 1
		Object obj = Math.random() < 1 ? "Hello World!" : 42;
		this.string = Math.random() < 1 ? obj.toString() : null;
		// this.string is now a JavaScript String object
	}
	
	@Override
	public String toString() {
		return this.string;
		// returned a JS String object and not a string primitive
		// -> the JS toString method will also return a String object
	}
	
}

Then pass an instance of that class to String.valueOf(Object):
String.valueOf(new StringObject())

String.valueOf(object) gets compiled to '' + object which expects object.toString() to return a string primitive.

When object is a String object, this throws a JavaScriptException: (TypeError) (at least in both Firefox 4 and Chromium 12)
Dec 23, 2011
Project Member #19 t.broyer
 Issue 3949  has been merged into this issue.
Dec 23, 2011
#20 cromwell...@gmail.com
We can fix the String.valueOf() method to force a coercion. For the JSNI issue, maybe an annotation? Clearly, most JSNI code runs fine and doesn't need the coercion. Something like

public native void foo(@JsString String foo, ...)

which will force a compiler generated coercion?

Dec 23, 2011
#21 smartgwt...@gmail.com
Because we generate the SmartGWT wrapper code, an annotation for JSNI methods wouldn't be too painful for us.  But it would be a lot better if it wasn't done in such a way that there was a compile-time dependency on a new GWT version.

However, just for background, I don't think there's ever a circumstance where it would be the right thing to pass a Java String as a String Object to JSNI code.  As far as I can tell, it's never desirable and always a gotcha.
Dec 23, 2011
#22 cromwell...@gmail.com
For most of the JSNI uses by the GWT SDK itself, I don't think it matters, for example,  you do not want every method like this:

public native void setAttribute(String name, String value) /*-{
  this.setAttribute(name, value);
}-*/;

which is inlined, to suddenly look like this:

this.setAttribute(String(name), String(value)); //(or name.valueOf())

it would just lead to massive bloat have all of this de-boxing code all around when for the most part, only JSNI wrappers for hand written libraries care (and it seems most of them don't even care).

The options I see are:

1) Don't modify Object.prototype/String.prototype. But this means every single virtual method dispatch site that calls methods on String will need a trampoline to check for String, which is a lot of bloat.

2) Make all String methods add a coercion (e.g. toString -> return this.valueOf()?)

3) Make all JSNI parameters get auto-coerced  (adds bloat to every DOM call for example)

4) Option #3 with annotation to switch it on





 


Dec 27, 2011
#23 smartgwt...@gmail.com
All of these options are quite bad.  Even if an annotation was available and we dealt with the pain of compile-time incompatibility with older GWT, apparently you're indicating this would be a runtime check and runtime conversion from String Object to String primitive, so if we used it pervasively it would slow down every call.

Option #0 would be avoiding ending up with String Objects in core GWT.  We know of no functional or performance benefits to having String Objects around..  scottb previously expressed puzzlement over how other JavaScript developers deal with String Object vs String primitive, and again the answer is, we don't use String Objects.  Any API in a JavaScript framework that was documented as returning String and actually returned a String Object would most likely be treated as a bug and resolved.

Using String primitives pervasively does not prevent installing instance methods on the JavaScript String object.  It only means that such instance methods can't return "this" and must instead return a String primitive.

If you're worried about this creating an allocation, a quick test reveals that the following approach correctly returns a String primitive instead of a String Object.  I'd be very surprised if any interpreters treated this as an allocation given that strings are immutable.

String.prototype.testMethod = function () { return this.toString(); };
typeof "foo".testMethod() // "string"
Jan 1, 2012
Project Member #24 t.broyer
Should I understand that having String.toString() return String(this) or this.toString(), as we talked about 2 years ago, be enough?

(as a side note, might I ask why you didn't tell it more loudly and submit a patch?)
Owner: cromwell...@google.com
Cc: -cromwell...@google.com -bobv%google.com@gtempaccount.com
Jan 2, 2012
#25 smartgwt...@gmail.com
If that's the only place where GWT ends up creating / returning String Objects, yes that should solve everything.  Sorry we didn't suggest it earlier, but we didn't realize this was the only place the String Objects were coming from.
Jan 3, 2012
Project Member #26 t.broyer
Given that java.lang.String is final, only methods on the emulated java.lang.String and its superclass java.lang.Object can have "return this" methods triggering the bug.

I checked the classes and there are a couple of places where that happens:
 - String#toString()
 - String#trim() in some specific case (defined by the trim() contract in the javadoc)
I guess it could also happen in String#split, which uses 'var trail = this', where the 'trail' could then be returned as-is in some cases (string is empty, regex doesn't match, or maxMatch==1)
Jan 3, 2012
Project Member #27 t.broyer
Actually, trim() and split() are not an issue because GWT devirtualizes them (as String is 'final' and the methods are defined on String, polymorphism doesn't apply; toString on the other hand is defined on java.lang.Object).

Also, note that the generated code has changed since GWT 2.0 (see comment #1 above): there no longer is an explicit check for the String type in the generated toString_devirtual$ method (when generated at all, which is only when you call toString on an Object that could be a JSO –i.e. that has not been proven at compile-time to never contain a JSO–; otherwise, a toString$ method is called on the object itself –even if it's been proven at compile-time to always be a String–)
Jan 3, 2012
Project Member #28 t.broyer
http://gwt-code-reviews.appspot.com/1623803
Status: ReviewPending
Labels: -Category-Compiler Category-JRE
Jan 3, 2012
#29 jat@google.com
Sorry, just getting on this late.  It has been a long time since I worked on this area, so maybe things are different, but in general you could get wrapped Strings pretty much arbitrarily from the browsers and it wasn't clear when.  Going through the DOM could wrap or unwrap them, looking up object properties (as the original example here), etc.  Putting in code everywhere to force all to be wrapped or all to be unwrapped is expensive, so as Scott mentioned the approach we took was to make sure that all the code GWT generated worked equally well with wrapped or unwrapped strings.  If JSNI code taking strings doesn't work equally well, then that can be addressed by putting the coercion in that JSNI code.

So, I am not sure agree with the patch -- is the value gained worth the cost for every existing user that doesn't pass Strings to JSNI code that cares about whether it is wrapped or not?
Jan 3, 2012
Project Member #30 cromwell...@google.com
We could add a configuration property for it. Although I'm not sure of any other cases where it would matter, since we don't override any other native prototypes to return 'this'.

We'd probably want an extra compiler pass to remove coercion when it is not needed, e.g.

String a = expr;
String b = expr2;
String c = a.toString() + b.toString();

We do not want this to be compiled with coercions, since the '+' operator should coerce the result if either side is a string? 

Jan 3, 2012
#31 smartgwt...@gmail.com
@jat - just wanted to jump in again to say that SmartClient uses browser DOM APIs pervasively and we are not aware of a case where you can obtain a String Object from the DOM.

Re this: "So, I am not sure agree with the patch -- is the value gained worth the cost for every existing user that doesn't pass Strings to JSNI code that cares about whether it is wrapped or not?"

Who do you mean?  It has been asserted that GWT Java developers can't tell, when working with a String, whether it's a String Object or not.  So who could be adversely affected by this change, and in what scenario?
Jan 3, 2012
#32 jat@google.com
Adding extra code to the execution path of every String.toString call seems like a bad thing that needs to be justified by the cost.

GWT doesn't create wrapped Strings itself, so the fact that you are seeing them at all and needing this fix means somewhere something is returning a wrapped String when you expect it isn't.

Regarding particular examples, when I first started working on DevMode 5 years ago, we found many cases where we could get a wrapped string when we expected a primitive, and that it varied across browsers.  At this point, I don't know that I could remember a specific example, other than what I have already said.  It is possible that it is no longer an issue, but even since then we had to add significant complexity to OOPHM in order to treat either a JS String primitive or JS String object as a Java String, because things failed before that code was added.
Jan 3, 2012
#33 smartgwt...@gmail.com
If you think there are ways that DOM calls can introduce String Objects, we are perfectly happy with a fix that just prevents String Objects being created every time someone calls String.toString() in GWT Java.  This would fix the bug that can be reproduced in pure GWT, and we believe it would also eliminate our JSNI problems.

As far as performance, as indicated above, there is a large optimization for IE6 you could implement by avoiding calling instance methods on Strings.  Anything else is frankly small potatoes by comparison, and I find it very plausible that this change has no measurable cost or is a small optimization.  After all, JavaScript VM implementers have probably paid a lot less attention to optimizing the obscure "String Object" as compared to the pervasive real String.
Jan 4, 2012
Project Member #34 t.broyer
@jat: have a look at the unit test in the review, GWT *does* create String objects (vs. String values) out of String#toString() when the compiler cannot optimize it out (i.e. when a String is stored in an Object variable that cannot be type-tightened to String).

Maybe using a 'return "" + this' (in either Java or JS) would be enough and have less impact on what the compiler can optimize?
Jan 4, 2012
Project Member #35 t.broyer
I tried with a 'return "" + this" in Java: the compiler optimizes it slightly better than the String(this) in JSNI. Notably, it prunes the ""+ when you concatenate the s.toString() with some other string. Other than that, there's no much change in the generated code, except that String(s) is 5 chars longer than ''+s.

Also, note that the overhead (compared to the current 'return this' code) is only:
 - once, in the toString$ method added to the String.prototype
 - each time toString() is called on something that is known at compile-time to be a String (because the method is then inlined, as it's then a static dispatch); unless the result is concatenated with some other string (so that "a"+s+"b" compiles to the same code as "a"+s.toString()+"b"; this is how ever only true if the 'return ""+this' variant is used); I guess the probability to find this case is rather low, so the overhead wouldn't be huge. If you're concerned about it though, an additional compiler optimization could be added to replace String#toString calls with the String instance itself (i.e. replace s.toString() with s when s is of type java.lang.String).

Speaking about optimizations, the empty string is "interned" into a global JS variable. In the case of the com.google.gwt.emultest.java.lang.StringTest, that variable always ended up with a 2-char-long name (iw or lw). That means it:
 - performs slower than ''+i (because it has to dereference a JS variable) for the same number of characters on the "call site"
 - wastes space by allocating an obfuscated identifier and having to declare the variable
I don't know if it would be easy to *not* "intern" the empty string, or always give it a one-char-long name.
Jan 4, 2012
Project Member #36 jat@jaet.org
Can you explain where GWT is creating a wrapped String object?

I don't think this is worth writing some extra compiler optimizations for.

Regarding interning the empty string, interning was found to be a big win on IE as it was much faster and reduces churn (IIRC, every string literal creates a new value while an interned reference just reuses the same one) -- at least through IE8, it still has horrible GC behavior (don't know if it was fixed in IE9).
Jan 4, 2012
Project Member #37 cromwell...@google.com
John, as I understand it, it is an artifact of the way JS VM's handle overriding native prototypes. Consider this (using number to make it more obvious):

Number.prototype.foo = function() { return 1; }

(42).foo()

The way the JS VM deals with this method invocation is to first wrap the primitive number, so in effect, you get this:

(new Number(42)).foo()

Now imagine this:

String.prototype.toString = function() { return this; }

Now writing:
("hello").toString() is in effect, written as this:

(new String("hello")).toString()

and now 'this' refers to the wrapped version.

Since the String object is the only JDK class that simultaneously overrides native prototypes AND returns 'this', it's the only place likely to cause the problem. 

I think we could make this totally optimal by having the compiler treat String.toString() (concrete, monomorphic, essentially static call) differently than Object.toString() (virtual, polymorphic)

I could add a pass to DeadCodeElimination to rewrite any essentially static call to String.toString() with 'this', and therefore, only in the case where the virtual one is called would you get "return String(this)", since only in the case of a virtual call does the problem even arise.

Jan 4, 2012
Project Member #38 jat@jaet.org
Ok, I retract my objection to changing toString's return value -- we can improve codegen later, unless you think it is trivial to do it now.  I didn't realize the JS VM would wrap it just because the prototype had a property set.
Jan 4, 2012
Project Member #39 scottb@google.com
When we originally went over this whole issue, we found that pure JS code, APIs, and DOM operations would use the wrapped and unwrapped String interchangably.  For example, we found that String property accesses on DOM elements would accept either one, and return either one.  It varied across browsers and even varied within the same browser.

Our conclusion was that generated GWT code had to be written in a way so as to expect either one at any time.  This is generally easy enough to do since handwritten JS generally needs to act the same way.  We did not make any effort to guarantee that all Strings are either wrapped or unwrapped as they move around in GWT code.

As a corollary, we've always said that JSNI code processing String values coming from GWT has to be written to accept either one.  If the JSNI code really cares, it has to do the coercion.
Jan 4, 2012
Project Member #40 cromwell...@google.com
What browsers did this occur on Scott? I wonder if any modern or recent browser returns non-primitive strings except in the well known case (prototype override)

Jan 4, 2012
Project Member #41 scottb@google.com
Dunno the details anymore.
Jan 4, 2012
#42 smartgwt...@gmail.com
Regarding scottb's assertion that pure JS code would use String Objects and String interchangeably, again, "typeof" returns "object" instead of "string" for a String Object, which means that a huge range of JavaScript sample code is broken if passed a String Object, as well as many APIs in common JS frameworks - just look at any open source one and grep for "typeof".

Re: talk about optimizations: I'm not sure why 'return "" + this" is being discussed, in Comment #23 I gave this approach, which seems just better:

----
If you're worried about this creating an allocation, a quick test reveals that the following approach correctly returns a String primitive instead of a String Object.  I'd be very surprised if any interpreters treated this as an allocation given that strings are immutable.

String.prototype.testMethod = function () { return this.toString(); };
typeof "foo".testMethod() // "string"
----
Jan 4, 2012
Project Member #43 cromwell...@google.com
We're not talking about allocations, we're talking about code size. I do not want something like this:

x.toString() + y.toString()

to turn into this:

String(x) + String(y)

Closure already performs this optimization, and we are adding the Closure backend to GWT, so it's a moot point.

The secondary optimization point is that GWT converts any call like String.toString()  (where the receiver is known to be of type String) to something like this:

public static native String toString$(String this$static) /*-{
  return String(this$static);
}-*/;

Here, the coercion is redundant because the typeof(this$static) == 'string', and it is a top level function now. 

Point being, String.toString() is used liberally in the JDK code base, and it is currently inlined by the JsInliner as 'this'. We want to avoid significant code bloat of having String(foo) inlined into thousands of callsites. 

A combination of the Closure backend, and special treatment for static calls to String.toString() would make things as optimal as possible.

Jan 4, 2012
#44 smartgwt...@gmail.com
Thanks for the deeper explanation.  Just to clarify, are you saying that you don't want to commit this fix until Closure has been added to GWT?

Not being familiar with the guts of JsInliner, it seems like special-casing String.toString() wouldn't be a massive undertaking..
Jan 4, 2012
Project Member #45 cromwell...@google.com
Actually, Closure is already included with -XenableClosureCompiler in the trunk, it just needs some fixes committed for code-splitting and symbolmaps/sourcemaps but otherwise works.

Jan 4, 2012
Project Member #46 t.broyer
@Ray: I already have the patch for DeadCodeElimination, the only reason I didn't updated the review with it yet is that tests don't run as expected: I don't understand why the virtual toString (String.prototype.toString$ when compiled in -style PRETTY, String.prototype.tS in -style OBF) turns into a 'return this' even after I reverted it to the JSNI code from my first patch (which I had verified to correctly compile to a 'return String(this)'). There must be some kind of caching somewhere...
I can update the code review nevertheless, if you want.

I was also thinking into special-casing the String.toString when translating to JS by simply assigning String.prototype.toString$ to String.prototype.toString instead of our own function (the toString in the String.java emulation would then never be used at all). We would then only lose stack-trace information, but it's highly unlikely that a 'return String(this)' would ever throw in this case. That would even save us a few bytes in the end (_.tS=_.toString vs. _.tS=function Nq(){return ''+this;} or similar)
Jan 4, 2012
Project Member #47 jat@jaet.org
When Java code calls obj.toString() and obj happens to have runtime type of String, how do you avoid calling the version in String.java?
Jan 4, 2012
Project Member #48 cromwell...@google.com
Thomas, it could be the unitCache, not sure. I'd do a clean rebuild and rm -rf the unitCache dirs to be sure.

I assume your optimization looks something like this: if methodCall is not virtual (static) and target == staticImplFor(String.toString) then replace with first arg of method call (this$static)


John, I'm not sure exactly what you mean, but there are the following scenarios:

obj.toString() is called, but 'obj' type is not known to be String, so it stays a virtual call, and ends up calling the super-source String.toString() which does 'return String(this)' coercion.

obj.toString() is called, but obj is type-tightened to String. Now the call is monomorphic (static), and implicitly a call to String.toString$(this$static) which does "return String(this$static)". But we add a pass to DeadCodeElimination which detects the static call to String.toString$(str) and replaces it with just 'str'.

So in every case where the compiler is dealing with not-provably-Java-string types, you have a virtual call which ends up invoking String.prototype.toString which contains a coercion. And in every other case, you end up with an inlined 'this'.

You can't avoid calling the version in String.toString() if the type is only known at runtime. But special casing the static dispatch would avoid bloat due to inlining of the static version.

You could debate how often the statically known String.toString() will be invoked, that will debate on type-tightening / call tightening effectiveness.

Feb 28, 2012
#49 EvanChar...@gmail.com
this is hitting us hard.  please fix ...

Evan Smith, MS MD
Apr 27, 2012
#50 bk.chock...@gmail.com
Same here. Any timetable for a fix?
May 15, 2012
Project Member #51 gwt.mirr...@gmail.com
This issue was closed by revision r10976.
Status: Fixed
May 15, 2012
Project Member #52 t.broyer
(No comment was entered for this change.)
Status: FixedNotReleased
Labels: Milestone-2_5
Jun 26, 2012
Project Member #53 skybr...@google.com
Bulk edit: should be fixed in the GWT 2.5 release candidate.

Status: Fixed
Sign in to add a comment

Powered by Google Project Hosting