[gwt-contrib] Re: code review requested, shard TypeSerializerCreator createMethodMap

2008-09-30 Thread Freeland Abbott 
Mostly LGTM.  The only (marginally) substantive complaint is that it may
make sense to rework the property parsing around lines 285-293 so that it's
done once, statically, rather than re-parsing the String-int conversion for
every TypeSerializer we create, especially if (as I suggest as a nit) you
change to use the System.getProperty(String propname, String default)
formulation.  Not that parsing a short string into int is any large amount
lot of work, but we may end up doing it often...

Nits

   - Can we note, probably in the comments around lines 58-66, that the
   shard size is measured in number of reachable types, not in e.g.
   characters?  (Yeah, I know characters would be hard to get at *a priori*,
   but since our issue seems to be string length of the JSNI body, that's what
   I expected size to be measured in.)
   - Also mentioned above, line 285-286, why avoid the
   System.getProperty(String prop, String default) formulation?  (You'd have to
   make the default a string, yes, but it saves you the if-null test.)
   - I'll take your word for the style-correctness of the added space on
   line 190.




On Tue, Sep 30, 2008 at 2:59 PM, John Tamplin [EMAIL PROTECTED] wrote:

 [fix bad GWTC address]


 Please review the attached patch for trunk, which fixes a hosted mode crash
 with a large set of serializable types.

 On Linux hosted mode, a very large list of serializable types can cause a
 crash inside Mozilla.  This appears to be related to the size of the JSNI
 function being evaluated to inject the code into the browser.

 This patch adds a Java system property to control this behavior, and code
 to split the createMethodMap if the numer of types exceeeds the specified
 threshold.  If no parameter is supplied, the output code is unchanged.
 After we get some feedback on exactly what this threshold should be, we may
 change it to a reasonable default.  This parameter may also go away once
 OOPHM is merged into trunk if it is no longer needed.  So, this should be
 considered a short-term patch.

 The relevant generated code previously looked like this:

 public static native JavaScriptObject createMethodMap() /*-{
   return {
 [Lcom.google.gwt.sample.dynatable.client.Person;/3792876907: [
 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::instantiate(Lcom/google/gwt/user/client/rpc/SerializationStreamReader;),

 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::deserialize(Lcom/google/gwt/user/client/rpc/SerializationStreamReader;[Lcom/google/gwt/sample/dynatable/client/Person;),

 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::serialize(Lcom/google/gwt/user/client/rpc/SerializationStreamWriter;[Lcom/google/gwt/sample/dynatable/client/Person;)
 ],
  ...
   };
 }-*/;

 it now looks like this if the threshold is exceeded:

 public static JavaScriptObject createMethodMap() {
   JavaScriptObject map = JavaScriptObject.createObject();
   createMethodMap_0(map);
   createMethodMap_50(map);
   ...
   return map;
 }

 public static native void createMethodMap_0(JavaScriptObject map) /*-{
   map[[Lcom.google.gwt.sample.dynatable.client.Person;/3792876907]=[
 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::instantiate(Lcom/google/gwt/user/client/rpc/SerializationStreamReader;),

 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::deserialize(Lcom/google/gwt/user/client/rpc/SerializationStreamReader;[Lcom/google/gwt/sample/dynatable/client/Person;),

 @com.google.gwt.sample.dynatable.client.Person_Array_Rank_1_FieldSerializer::serialize(Lcom/google/gwt/user/client/rpc/SerializationStreamWriter;[Lcom/google/gwt/sample/dynatable/client/Person;)
   ];
 }-*/;

 Note that the resulting code will be slightly larger and slower than
 before, so this should only be used for Linux hosted mode where needed.
 Since the default behavior is unchanged, if you do nothing there will be no
 impact on the output.

 --
 John A. Tamplin
 Software Engineer (GWT), Google


--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---

[gwt-contrib] Re: code review requested, shard TypeSerializerCreator createMethodMap

2008-09-30 Thread Freeland Abbott 
On Tue, Sep 30, 2008 at 4:27 PM, John Tamplin [EMAIL PROTECTED] wrote:

 On Tue, Sep 30, 2008 at 3:49 PM, Freeland Abbott 
 [EMAIL PROTECTED] wrote:

 Mostly LGTM.  The only (marginally) substantive complaint is that it may
 make sense to rework the property parsing around lines 285-293 so that it's
 done once, statically, rather than re-parsing the


 Ok.  I can't really do it in a static intiializer since I may need a
 TreeLogger, but I can only do it the first time.


Or you'd have to introduce a new static method, and call that from somewhere
early (i.e. after we have a TreeLogger, but before we're doing anything
that might loop over calls).




- Can we note, probably in the comments around lines 58-66, that the
shard size is measured in number of reachable types, not in e.g.
characters?  (Yeah, I know characters would be hard to get at *a priori
*, but since our issue seems to be string length of the JSNI body,
that's what I expected size to be measured in.)

 The comment for DEFAULT_CREATEMETHODMAP_SHARD_SIZE currently says:

 Default number of types to split createMethodMap entries into.  Zero means
 no sharding occurs.


 How are you suggesting that be rewritten?


Nevermind. :-/  That answers it; I just misread.



- Also mentioned above, line 285-286, why avoid the
System.getProperty(String prop, String default) formulation?  (You'd have 
 to
make the default a string, yes, but it saves you the if-null test.)

 So you would prefer doing:
 String shardSizeProperty =
 System.getProperty(GWT_CREATEMETHODMAP_SHARD_SIZE,
 String.valueOf(DEFAULT_CREATEMETHODMAP_SHARD_SIZE));


Yes, I like this better than the set-to-default-int, fetch-property,
test-for-null sequence.  Though my formulation had converted default to
String, rather than using String.valueOf(), which gets us to your next
comment...

This would always require converting the default to a string (or storing it
 as a string and losing type safety) and always parsing the result rather
 than a test telling you the parsing is needed.  It doesn't seem to be an
 improvement to me.


There's a reason it's in the nits category, and if you think it's a lose,
I don't mind if you skip the suggestion.

But, to make the case: I see one all-String code path as cleaner than two
paths with different data data types on each fork.  I'll grant that the
integer-fork else of your if is degenerate, but it takes a couple bytes
of code size to do the test.  Equally, surely it's not a real problem to
ensure that the compiled-in default is in fact an integer (that is, the
used-once compiled-in string's type safety is a small issue, even among
small issues), and we need to handle non-integer values gracefully anyway
(because the user can override).

But, again, it's a nit: my way saves at most ~10b of code size and could
gain or lose a few miliseconds in execution time depending on whether branch
prediction in the JVM is so far below bytecode interpretation that misses
appear free; your way is type-safe for a constant that's both clearly
documented as an integral value and likely to change exactly once in its
coded lifetime.  What color dresses are the angels on that pin wearing?

--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---

[gwt-contrib] Re: code review requested, shard TypeSerializerCreator createMethodMap

2008-09-30 Thread John Tamplin 
On Tue, Sep 30, 2008 at 4:51 PM, Freeland Abbott [EMAIL PROTECTED]
 wrote:

 But, again, it's a nit: my way saves at most ~10b of code size and could
 gain or lose a few miliseconds in execution time depending on whether branch
 prediction in the JVM is so far below bytecode interpretation that misses
 appear free; your way is type-safe for a constant that's both clearly
 documented as an integral value and likely to change exactly once in its
 coded lifetime.  What color dresses are the angels on that pin wearing?


Ok, try this patch.

-- 
John A. Tamplin
Software Engineer (GWT), Google

--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---

Index: user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java
===
--- user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java	(revision 3693)
+++ user/src/com/google/gwt/user/rebind/rpc/TypeSerializerCreator.java	(working copy)
@@ -19,6 +19,7 @@
 import com.google.gwt.core.client.JavaScriptObject;
 import com.google.gwt.core.ext.GeneratorContext;
 import com.google.gwt.core.ext.TreeLogger;
+import com.google.gwt.core.ext.UnableToCompleteException;
 import com.google.gwt.core.ext.typeinfo.JClassType;
 import com.google.gwt.core.ext.typeinfo.JMethod;
 import com.google.gwt.core.ext.typeinfo.JParameterizedType;
@@ -32,6 +33,8 @@
 import com.google.gwt.user.rebind.SourceWriter;
 
 import java.io.PrintWriter;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * This class generates a class with name 'typeSerializerClassName' that is able
@@ -52,6 +55,39 @@
   + SerializationStreamWriter streamWriter, Object instance, String typeSignature)
   +  throws SerializationException;
 
+  /**
+   * Default number of types to split createMethodMap entries into.  Zero means
+   * no sharding occurs.  Stored as a string since it is used as a default
+   * property value.
+   */
+  private static final String DEFAULT_CREATEMETHODMAP_SHARD_SIZE = 0;
+
+  /**
+   * Java system property name to override the above.
+   */
+  private static final String GWT_CREATEMETHODMAP_SHARD_SIZE = gwt.typecreator.shard.size;
+
+  private static int shardSize = -1;
+
+  private static void computeShardSize(TreeLogger logger)
+  throws UnableToCompleteException {
+String shardSizeProperty = System.getProperty(
+GWT_CREATEMETHODMAP_SHARD_SIZE, DEFAULT_CREATEMETHODMAP_SHARD_SIZE);
+try {
+  shardSize = Integer.valueOf(shardSizeProperty);
+  if (shardSize  0) {
+logger.log(TreeLogger.ERROR, GWT_CREATEMETHODMAP_SHARD_SIZE
++  must be non-negative:  + shardSizeProperty);
+throw new UnableToCompleteException();
+  }
+} catch (NumberFormatException e) {
+  logger.log(TreeLogger.ERROR, Property 
+  + GWT_CREATEMETHODMAP_SHARD_SIZE +  not a number: 
+  + shardSizeProperty, e);
+  throw new UnableToCompleteException();
+}
+  }
+
   private final GeneratorContext context;
 
   private final JType[] serializableTypes;
@@ -66,7 +102,7 @@
 
   public TypeSerializerCreator(TreeLogger logger,
   SerializableTypeOracle serializationOracle, GeneratorContext context,
-  String typeSerializerClassName) {
+  String typeSerializerClassName) throws UnableToCompleteException {
 this.context = context;
 this.typeSerializerClassName = typeSerializerClassName;
 this.serializationOracle = serializationOracle;
@@ -75,9 +111,14 @@
 serializableTypes = serializationOracle.getSerializableTypes();
 
 srcWriter = getSourceWriter(logger, context);
+if (shardSize  0) {
+  computeShardSize(logger);
+}
+logger.log(TreeLogger.TRACE, Using a shard size of  + shardSize
++  for TypeSerializerCreator createMethodMap);
   }
 
-  public String realize(TreeLogger logger) {
+  public String realize(TreeLogger logger) throws UnableToCompleteException {
 logger = logger.branch(TreeLogger.DEBUG,
 Generating TypeSerializer for service interface '
 + getTypeSerializerClassName() + ', null);
@@ -92,7 +133,7 @@
 
 writeCreateMethods();
 
-writeCreateMethodMapMethod();
+writeCreateMethodMapMethod(logger);
 
 writeCreateSignatureMapMethod();
 
@@ -173,13 +214,13 @@
   packageName = className.substring(0, index);
   className = className.substring(index + 1, className.length());
 }
-return new String[]{packageName, className};
+return new String[] {packageName, className};
   }
-  
+
   private JType[] getSerializableTypes() {
 return serializableTypes;
   }
-  
+
   private SourceWriter getSourceWriter(TreeLogger logger, GeneratorContext ctx) {
 String name[] = getPackageAndClassName(getTypeSerializerClassName());
 String packageName = name[0];
@@ -207,6 +248,16 @@
   }
 
   /**
+   * @param type
+   * @return
+   */
+

[gwt-contrib] Re: code review requested, shard TypeSerializerCreator createMethodMap

2008-09-30 Thread Freeland Abbott 
LGTM

On Tue, Sep 30, 2008 at 5:26 PM, John Tamplin [EMAIL PROTECTED] wrote:

 On Tue, Sep 30, 2008 at 4:51 PM, Freeland Abbott 
 [EMAIL PROTECTED] wrote:

 But, again, it's a nit: my way saves at most ~10b of code size and could
 gain or lose a few miliseconds in execution time depending on whether branch
 prediction in the JVM is so far below bytecode interpretation that misses
 appear free; your way is type-safe for a constant that's both clearly
 documented as an integral value and likely to change exactly once in its
 coded lifetime.  What color dresses are the angels on that pin wearing?


 Ok, try this patch.

 --
 John A. Tamplin
 Software Engineer (GWT), Google


--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---