Proposed API change: JavaScriptObject.createEmptyObject() + public FastStringMap

[Scott Blum]

Hi all,

This is related to issue #631.  While a final resolution of #631 is still in debate (ie, using hasOwnProperty), I'd like to make a change that will suffice for most uses and alleviate some of the worst problems.  Hopefully the doc below is self-explanatory; it would be used in a few different places, such as JSONObject, String.hashCache, and ClientSerializationStreamWriter.  Additionally, I thought I'd propose a public version of FastStringMap that does not implement the Map interface.  I feel that the semantics of the Map interface are almost impossible to get correct with retaining efficiency, and largely unnecessary for many purposes.  The second item I'm not super passionate about getting in right this second, but I know it's been asked for.

Doc is inlined, files are attached.

Scott

package com.google.gwt.core.client;

public class JavaScriptObject {

  /**
   * Returns a JavaScriptObject suitable for use as an associative map.
   * Specifically, if <code>o</code> is the returned object and <code>x</code>
   * is any string value, <code>o[x]</code> will initially evaluate to
   * <code>undefined</code>.
   * <p>
   * The returned object may not be suited for general purpose use. Expected
   * operations, such as <code>o.toString()</code>, will not work because
   * <code>o.toString</code> is initially undefined.
   * </p>
   * <p>
   * Iterating over the object's fields using the 'in' keyword is NOT guaranteed
   * to include every property that has been set. In addition, such iteration
   * may include properties which have not been explicitly set but whose value
   * is undefined.
   * </p>
   */
  public static JavaScriptObject createEmptyObject();

}

package com.google.gwt.core.client;

/**
 * A really fast map that only supports String keys.
 */
public class FastStringMap {
  /*
   * It would be supremely useful to have overloads for every primitive type, so
   * that primitives could be used without boxing. However, it may be better to
   * just wait until Java 5.0 support and use generics. Specific overloads for
   * JavaScriptObject may also be necessary.
   */

  public void clear();

  public boolean containsKey(String key);

  public Object get(String key);

  public void put(String key, Object value);

  public Object remove(String key);

}

[Rob Jellinghaus]

On Mar 16, 1:24 pm, "Scott Blum" <sco...@google.com> wrote:
> Additionally, I thought I'd propose a
> public version of FastStringMap that does not implement the Map interface.
> I feel that the semantics of the Map interface are almost impossible to get
> correct with retaining efficiency, and largely unnecessary for many
> purposes.

Can you be more specific? What is the efficiency issue? (Low-
priority question asked out of curiosity, disregard if too busy.)

Cheers!
Rob

[Bruce Johnson]

I'm all for createEmptyObject, but we should table FastStringMap.

These are two independent API changes that would best be discussed separately. Including FastStringMap is creeping featurism for 1.4. People who need it can easily copy/paste it from the existing package-protected code. I only advocate copy/paste because the only thing worse than copy/paste is having half-baked APIs, and I think that's what we'd end up with. There is very likely a whole set of super lightweight collections waiting to be born, but I'd rather cluster FastStringMap with that (at least then we can pick the right package for it).

[Emily Crutcher]

Also, with the new HashMap implementation, it's not clear whether we even want FastStringMap anymore, in fact I was planning to switch HTMLTable back to using HashMap in the near future.

[Toby Reyelts]

Yes, I'm slightly confused. I thought our new HashMap impl special-cased Strings, so you'd generally get the speed of "FastStringMap". It thought it was TreeMap that we were having troubles fully supporting the semantics in an "optimized" fashion.

[Emily Crutcher]

Yes, that is correct.  HashMap has to do a bit more casting and instanceofs, which add a small overhead. But overall, I don't think it's worth keeping FastStringMap.

[Scott Blum]

Fair enough.  Consider it dropped.

On 3/16/07, Bruce Johnson <br...@google.com> wrote:

[Sandy McArthur]

On 3/16/07, Scott Blum <sco...@google.com> wrote:

> This is related to issue #631. While a final resolution of #631 is still in
> debate (ie, using hasOwnProperty), I'd like to make a change that will
> suffice for most uses and alleviate some of the worst problems. Hopefully
> the doc below is self-explanatory; it would be used in a few different
> places, such as JSONObject, String.hashCache, and
> ClientSerializationStreamWriter.

-1 (non-binding)
Because this approach to solving 631 is not as future-proof as using
hasOwnProperty or a prefix method I think this is a poor addition to
GWT.

New JavaScript engine updates could break exsting GWT apps because the
"killWords" can get out of date. Mozilla has announced that they will
be reworking their JavaScript engine to take advantage of the
JavaScript JIT donated by Adobe. Who knows what internal details will
change with that. Also, as browsers support new versions of ECMAScript
the list of needed killWords may change.

I don't want to potentially revisit and rebuild old GWT apps to try to
diagnose very obscure bugs every few years. Every employer out there
is going to feel the same way.

--
Sandy McArthur

"He who dares not offend cannot be honest."
- Thomas Paine

[Scott Blum]

On 3/17/07, Sandy McArthur <sand...@gmail.com> wrote:

-1 (non-binding)
Because this approach to solving 631 is not as future-proof as using
hasOwnProperty or a prefix method I think this is a poor addition to
GWT.

New JavaScript engine updates could break exsting GWT apps because the
"killWords" can get out of date. Mozilla has announced that they will
be reworking their JavaScript engine to take advantage of the
JavaScript JIT donated by Adobe. Who knows what internal details will
change with that. Also, as browsers support new versions of ECMAScript
the list of needed killWords may change.

This is fair criticism.  Here's how I'll defend this change:
1) It's better than what is there now.
2) It's not worst than what is there now (contrast with using a prefix, which would be slower).
3) It's doable for 1.4, with a minimal impact on existing code.

Now, you may be right that a hasOwnProperty style solution is ultimately better, assuming it works on all the browsers we care about.  However, it will required changing all the use sites (and yes, I know you've got a patch in for this), which adds risk of introducing a bug in one of those sites.  I think this is a reasonable stop-gap solution as well as a useful partial.

I don't want to potentially revisit and rebuild old GWT apps to try to
diagnose very obscure bugs every few years. Every employer out there
is going to feel the same way.

Sure, I get that.  One counterpoint, though, is that every JavaScript program potentially has this problem, and ones like it.  How many times have I seen handwritten JavaScript that checks for the existence of a function to do one thing or another (poor man's browser detection).  When a later version of the browser has that function, but perhaps with different-than-expected semantics, things go boom.  I'm not saying it's a perfect solution, but I do think it's a good solution.

Scott

[Bruce Johnson]

Just a thought: but is there any way to infer the list of kill words
at runtime? Could we, for example, use for..in as way to just blindly
enumerate and undefine anything that's there?

[Scott Blum]

We might want to add this to the implementation as a safeguard, but in general this won't work.  None of the default properties are enumerable on any browser I tested.

[Scott Blum]

Seeing that there's been no movement on this issue for a few days, I'm
going to put a patch together if there are no furthur objections. "We
should do something better" is a very valid point, but I don't think
it means "We should not go ahead and do something that's better than
what we've already got."

[Kelly Norton]

I actually misread this thread and thought the patch was fully tabled, but I'm guessing that only FastHashMap was tabled? Otherwise, I would have weighed in earlier.

I'm ok with putting in a band-aid solution, but what's our timeline for really fixing it? Is this just something to put in place for the feature preview that will be fixed more completely before the RC? I think I might be in favor of that. The only thing that concerns me is that we're putting in a solution we're really not happy with, which will fix the tests, which will remove the impetus for a proper fix. And I definitely don't think it should close issue #631.

The main thing that concerns me about the kill-words approach is not so much the future proofing issue, but the implementation specific property issue. The list of standard properties varies across browser as some implementations have used non-standard properties to expose implementation specifics. For instance, your list is missing '__proto__' which is a FF only property holding an instance's prototype chain.

/kel

[Bruce Johnson]

Kelly makes a good point. I suppose as long as the issue remains open, we can go back and look. But, like we're discussing on John's IE7 hack, it's risky to do a incomplete solution that might make us forget about the true problem.

Pragmatically, it's fine with me to go ahead, but please make sure the issue is marked either High or Critical priority for 1.4 first.

[Scott Blum]

On 3/23/07, Kelly Norton <kno...@google.com> wrote:

> I'm ok with putting in a band-aid solution, but what's our timeline for
> really fixing it? Is this just something to put in place for the feature
> preview that will be fixed more completely before the RC? I think I might be
> in favor of that. The only thing that concerns me is that we're putting in a
> solution we're really not happy with, which will fix the tests, which will
> remove the impetus for a proper fix. And I definitely don't think it should
> close issue #631.

Yes, I was definitely NOT planning to close issue #631. I do think
it's a useful enough partial to merit fixing some of the immediate
problems.

> The main thing that concerns me about the kill-words approach is not so much
> the future proofing issue, but the implementation specific property issue.
> The list of standard properties varies across browser as some
> implementations have used non-standard properties to expose implementation
> specifics. For instance, your list is missing '__proto__' which is a FF only
> property holding an instance's prototype chain.

We should definitely make to be as thorough as possible, consulting
browser doc, etc. Where did you find the doc for __proto__? I was
looking here (http://devedge-temp.mozilla.org/library/manuals/2000/javascript/1.5/reference/object.html)
which doesn't list it.

Scott

[Sandy McArthur]

On 3/23/07, Scott Blum <sco...@google.com> wrote:

> On 3/23/07, Kelly Norton <kno...@google.com> wrote:
> > I'm ok with putting in a band-aid solution, but what's our timeline for
> > really fixing it? Is this just something to put in place for the feature
> > preview that will be fixed more completely before the RC? I think I might be
> > in favor of that. The only thing that concerns me is that we're putting in a
> > solution we're really not happy with, which will fix the tests, which will
> > remove the impetus for a proper fix. And I definitely don't think it should
> > close issue #631.
>
> Yes, I was definitely NOT planning to close issue #631. I do think
> it's a useful enough partial to merit fixing some of the immediate
> problems.

Should a potentially band-aid solution expose a new public method?

> > The main thing that concerns me about the kill-words approach is not so much
> > the future proofing issue, but the implementation specific property issue.
> > The list of standard properties varies across browser as some
> > implementations have used non-standard properties to expose implementation
> > specifics. For instance, your list is missing '__proto__' which is a FF only
> > property holding an instance's prototype chain.
>
> We should definitely make to be as thorough as possible, consulting
> browser doc, etc. Where did you find the doc for __proto__? I was
> looking here (http://devedge-temp.mozilla.org/library/manuals/2000/javascript/1.5/reference/object.html)
> which doesn't list it.

http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Property_Inheritance_Revisited:Determining_Instance_Relationships

When I add "__proto__" to the kw array from your foo.html tester I get
the alert "__proto__ is defined!" in Firefox. And the "done" alert
never shows because of a script error in the "// make sure the
assignment stuck" tests.
http://groups.google.com/group/Google-Web-Toolkit-Contributors/msg/eb3e5cee315a4e25

Unfortunately in my tests hasOwnProperty("__proto__") always returns
true for me in Firefox. This makes the prefix method the only reliable
method which only works for the Map use cases. The places where a user
provided JSO is used (Dictionary, JSON) you have to use hasOwnProperty
plus a special case check for "__proto__" in gecko browsers to be
correct.

[Scott Blum]

On 3/23/07, Sandy McArthur <sand...@gmail.com> wrote:

Should a potentially band-aid solution expose a new public method?

If the method is itself inherently useful, I think the answer is yes.  I do think it's inherently useful, personally.

http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Guide:Property_Inheritance_Revisited:Determining_Instance_Relationships

Thanks.

When I add "__proto__" to the kw array from your foo.html tester I get
the alert  "__proto__ is defined!" in Firefox. And the "done" alert
never shows because of a script error in the "// make sure the
assignment stuck" tests.
http://groups.google.com/group/Google-Web-Toolkit-Contributors/msg/eb3e5cee315a4e25

Unfortunately in my tests hasOwnProperty("__proto__") always returns
true for me in Firefox. This makes the prefix method the only reliable
method which only works for the Map use cases. The places where a user
provided JSO is used (Dictionary, JSON) you have to use hasOwnProperty
plus a special case check for "__proto__" in gecko browsers to be
correct.

Rats, that is indeed unfortunate.  If that's the only case, though, I'm not sure I would consider it a dealbreaker.  Thoughts?
 

The case where the user provides the JSO to JSON seems ambiguous to me... are we going for a true reflection of their object (and all its warts), or some kind of scrubbed view of it?

Scott

[Sandy McArthur]

On 3/23/07, Scott Blum <sco...@google.com> wrote:

On 3/23/07, Sandy McArthur <sand...@gmail.com> wrote:

Should a potentially band-aid solution expose a new public method?

If the method is itself inherently useful, I think the answer is yes.  I do think it's inherently useful, personally.

People don't have to use it if they don't want to so I don't have a strong objection against it beyond my bias against additional public API and that it will lull developers into a false sense of security because it's never going to be reliable.

People do have to use Maps and existing web services so I do have strong opinions on those correctly.

Unfortunately in my tests hasOwnProperty("__proto__") always returns
true for me in Firefox. This makes the prefix method the only reliable
method which only works for the Map use cases. The places where a user
provided JSO is used (Dictionary, JSON) you have to use hasOwnProperty
plus a special case check for "__proto__" in gecko browsers to be
correct.

Rats, that is indeed unfortunate.  If that's the only case, though, I'm not sure I would consider it a dealbreaker.  Thoughts?

The first two paragraphs of the GWT front page, I added the emphasis:

Google Web Toolkit (GWT) is an open source Java software development framework that makes writing AJAX applications like Google Maps and Gmail easy for developers who don't speak browser quirks as a second language . Writing dynamic web applications today is a tedious and error-prone process; you spend 90% of your time working around subtle incompatibilities between web browsers and platforms, and JavaScript's lack of modularity makes sharing, testing, and reusing AJAX components difficult and fragile.

GWT lets you avoid many of these headaches while offering your users the same dynamic, standards-compliant experience. You write your front end in the Java programming language, and the GWT compiler converts your Java classes to browser-compliant JavaScript and HTML. 

The very first piece of copy for GWT emphasizes how it will make the developer's lives easier by trying to take care of quirks and browser bugs for them. The words "fast", "speed", or "quick" do not appear on the front page but that seems to be the primary factor for consideration.

If GWT 1.4.rc7 was release and you're desperately trying to get to a "stable" release then I'd have more empathy for the concern that using a clean object is a less intrusive change where you can use it. But GWT 1.4.rc1 isn't even out yet so you have an opportunity to get some real testing on the most intrusive but more robust changes.

The case where the user provides the JSO to JSON seems ambiguous to me... are we going for a true reflection of their object (and all its warts), or some kind of scrubbed view of it?

I think it's reasonable to get out of a JSON object only what was put into it:

// evalJavaScript(String) is JavaScript's eval on a string
JavaScriptObject jso = evalJavaScript("{ 'foo': 'bar' }");

JSONObject json = new  JSONObject(jso);

// should only be true when the JSON response actually contains a "watch" property
if (json.containKey("watch")) {
   throw new UnexpectedBehaviorException();
}

Now what if jso came from a JSON service via XmlHttpRequst? It could include anything and unless you force people to use your own custom JSON evaluator you cannot make sure clean objects are used. What if jso came from a legacy pure javascript lib that was called via JSNI?

Dictionary similar because it expected people to create the JSO in a script tag in their host page. The docs for Dictionary even give an example telling people to do this.

[Bruce Johnson]

Sandy is making a number of persuasive points. I think we should reconsider createEmptyObject() unless we can identify some sort of once-and-for-all solution that addresses all of the concerns (which, unfortunately, doesn't appear to be possible).

-- Bruce

[John Tamplin]

On 3/26/07, Bruce Johnson <br...@google.com> wrote:

Sandy is making a number of persuasive points. I think we should reconsider createEmptyObject() unless we can identify some sort of once-and-for-all solution that addresses all of the concerns (which, unfortunately, doesn't appear to be possible).

As I understand it, the prefix approach would not change any APIs, would be guaranteed (well, almost) correct, and would be easy to implement -- could we not use if for now and then work on a faster approach later?  Or is the size/speed penalty simply too severe to stomach?

--
John A. Tamplin
Software Engineer, Google

[Sandy McArthur]

Kelly wrote this micro-benchmark a while back:
http://web.kellegous.com/scratch/2007/01-hash-bm/

I've made a table comparing the four browsers:
http://spreadsheets.google.com/pub?key=pDH5-1uJePizcHBcYRipLIw

Generally speaking using a prefix is 3.8% slower than trying to use a
clean object when it comes to raw inserts and reads. I suspect the
worst case would be iterating over a Map's keys because you'd have to
strip the ':' prefix every time.

Still, that ~4% is in the context of a micro-benchmark. In the context
of a whole app, how much time is really spent inside the Map classes?
I can't really answer that in web mode without a profiling tool but in
the Java app I'm working on right now my profiler rounds it to 0% of
total cpu time. 4% of at most 0.4999% isn't much. Try the more robust
approach first, and if the groups catch on fire from Maps being
intolerably slow then try something else.

[Ian Petersen]

On 3/26/07, Sandy McArthur <sand...@gmail.com> wrote:
> Still, that ~4% is in the context of a micro-benchmark. In the context
> of a whole app, how much time is really spent inside the Map classes?
> I can't really answer that in web mode without a profiling tool but in
> the Java app I'm working on right now my profiler rounds it to 0% of
> total cpu time. 4% of at most 0.4999% isn't much. Try the more robust
> approach first, and if the groups catch on fire from Maps being
> intolerably slow then try something else.

That sounds like something Knuth might say in reference to
optimization and evil.... I like it.

Ian

--
Tired of pop-ups, security holes, and spyware?
Try Firefox: http://www.getfirefox.com

[John Tamplin]

On 3/26/07, Sandy McArthur <sand...@gmail.com> wrote:

Still, that ~4% is in the context of a micro-benchmark. In the context
of a whole app, how much time is really spent inside the Map classes?
I can't really answer that in web mode without a profiling tool but in
the Java app I'm working on right now my profiler rounds it to 0% of
total cpu time. 4% of at most 0.4999% isn't much. Try the more robust
approach first, and if the groups catch on fire from Maps being
intolerably slow then try something else.

What about the size penalty?  If it prevents methods from being inlined, or if those inlined methods grow much larger, it could be a pretty substantial size penalty.