Conditional compilation

71 views
Skip to first unread message

Alex Tkachman

unread,
Feb 11, 2007, 4:21:57 PM2/11/07
to Google Web Toolkit Contributors
There are many well-know cases when conditional compilation may be
really useful. Debug vs. Production is probably most popular example.

I want to note that GWT Generators follow exactly (or almost) the same
pattern - properties defined for permutation dictate what code to
compile. For example, in one of my applications (patch to GWT :) in
progress) I use property provider, which check application URL for
presence of special query attribute, and generate either debug or
production version of my classes. It works but inconvinient.

What I would love to be able to write is

if ( GWT.getBooleanProperty("debug",false) )
{
// debug code
}

Formal definition for GWT.getBooleanProperty is

public static boolean getBooleanProperty (String propertyName, boolean
defaultValue)

Of course, I expect compiler to be smart enough to replace this call
either by true or false depending on property value and default value.
After that regular dead code elimination process will do the work.

Obviously, we can imagine also GWT.getStringProperty,
GWT.getIntegerProperty etc. This functionality can become extremely
powerful if compiler is able to simplify expressions like <literal>
<binary operation> <literal> to <new literal> and especially if it is
able to evaluate methods like Sting.equals, Sting.equalsIgnoreCase,
Sting.indexOf etc. with parameters known at runtime.

Does such API extension make any sense?

Scott Blum

unread,
Feb 11, 2007, 6:57:44 PM2/11/07
to Google-Web-Tool...@googlegroups.com
Perhaps so, especially as our compiler becomes better (and better at
optimizing). Before such an API wouldn't have helped much due to lack
of good dead code elimination, but now (thanks, Sandy!) I could see
some real value here.

Emily Crutcher

unread,
Feb 12, 2007, 6:32:02 AM2/12/07
to Google-Web-Tool...@googlegroups.com
This seems like a great feature to have, though I'd still love to see the assert mechanism as well...
 

Alex Tkachman

unread,
Feb 12, 2007, 6:35:46 AM2/12/07
to Google Web Toolkit Contributors
I am working on both right now.
Interesting that most complicated part for me was to delivery
PropertyOracle to places where we need it.

On Feb 12, 2:32 pm, "Emily Crutcher" <e...@google.com> wrote:
> This seems like a great feature to have, though I'd still love to see the
> assert mechanism as well...
>
> On 2/11/07, Scott Blum <sco...@google.com> wrote:
>
>
>
>
>
> > Perhaps so, especially as our compiler becomes better (and better at
> > optimizing). Before such an API wouldn't have helped much due to lack
> > of good dead code elimination, but now (thanks, Sandy!) I could see
> > some real value here.
>

> --
> "There are only 10 types of people in the world: Those who understand
> binary, and those who don't"

Alex Tkachman

unread,
Feb 12, 2007, 9:50:20 AM2/12/07
to Google-Web-Tool...@googlegroups.com
Scott!

Could you do me a favor and have a look for my very first draft of
patch for conditional compilation.

Best regards
Alex

Compile_time_properties_-_first_draft.patch

Fred Sauer

unread,
Feb 12, 2007, 12:57:39 PM2/12/07
to Google-Web-Tool...@googlegroups.com
On 2/11/07, Alex Tkachman <alex.t...@gmail.com> wrote:

There are many well-know cases when conditional compilation may be
really useful. Debug vs. Production is probably most popular example.
[...]

What I would love to be able to write is

if ( GWT.getBooleanProperty ("debug",false) )
{
  // debug code
}


How about:
public class MyConstants {
  public static final boolean DEBUG = true;
}

and then:
if (MyConstants.DEBUG) {
  // debug code
}


Or, to follow Log4J, but with a "static final" qualifier for the current log level:
public class Logging {

  public static final int OFF = Integer.MAX_VALUE;
  public static final int FATAL = 50000;
  public static final int ERROR = 40000;
  public static final int WARN = 30000;
  public static final int INFO = 20000;
  public static final int DEBUG = 10000;

  /**
   * Current log level for this application with <code>final</code>
   * qualifier to ensure GWT compiler optimization, including
   * dead code removal.
   */
  public static final int LEVEL = WARN;

  public static void fatal(String message, Throwable e) {
    if (LEVEL <= FATAL) {
      log("FATAL", message, e);
    }
  }
 
  public static void error(String message, Throwable e) {
    if (LEVEL <= ERROR) {
      log("ERROR", message, e);
    }
  }

  public static void warn(String message) {
    if (LEVEL <= WARN) {
      log("WARN", message, null);
    }
  }
 
  public static void info(String message) {
    if (LEVEL <= INFO) {
      log("INFO", message, null);
    }
  }

  public static void debug(String message) {
    if (LEVEL <= DEBUG) {
      log("DEBUG", message, null);
    }
  }

  public static boolean isFatalEnabled() {
    return LEVEL <= FATAL;
  }
 
  public static boolean isErrorEnabled() {
    return LEVEL <= ERROR;
  }
 
  public static boolean isWarnEnabled() {
    return LEVEL <= WARN;
  }
 
  public static boolean isInfoEnabled() {
    return LEVEL <= INFO;
  }
 
  public static boolean isDebugEnabled() {
    return LEVEL <= DEBUG;
  }
 
  private static void log(String level, String message, Throwable e) {
    GWT.log("[" + level + "] " + message, e);
  }

}


and then in your code you can do each of the following, all of which get optimized away by dead code elimination:
Logging.debug("debug message here");

Logging.info("informational message here");

if (Logging.isDebugEnabled()) {
  // debug code
}

try {
  // do something which might fail
} catch(Exception e) {
  Logging.error("Oh no!", e);
  // possibly do something else with exception here
}

The above gets you global logging levels. If you want something more granular (say by package or by class), you can probably achieve that with multiple 'static final' logging level constants.

Hope this helps.


Sandy McArthur

unread,
Feb 12, 2007, 1:27:08 PM2/12/07
to Google-Web-Tool...@googlegroups.com
On 2/12/07, Fred Sauer <fr...@allen-sauer.com> wrote:
> How about:
> public class MyConstants {
> public static final boolean DEBUG = true;
> }
>
> and then:
> if (MyConstants.DEBUG) {
> // debug code
> }

When I first read Alex's proposal I first though this is really
another for of GWT's I18N but instead of Local based it's based on
some other parameter. Alex's proposal introduces more magic methods
beyond GWT.create which I don't like.

I think I'd rather see a means to plug in your own JJS optimizers
which can look for higher level things like like Alex wants and
optimize them. But that would make the AST a public interface and that
is undesirable. IntelliJ IDEA has a structural search and replace
feature which understands the code in an abstract way. Maybe something
like that could be the public API so the AST can remain a private API.
I would have to think on it more.

--
Sandy McArthur

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

Alex Tkachman

unread,
Feb 12, 2007, 1:48:44 PM2/12/07
to Google-Web-Tool...@googlegroups.com
Main difference with mine proposal is ability to compile several
versions of the same code and make them available for end user.

On 2/12/07, Fred Sauer <fr...@allen-sauer.com> wrote:
>

Fred Sauer

unread,
Feb 12, 2007, 7:06:56 PM2/12/07
to Google-Web-Tool...@googlegroups.com

On 2/12/07, Alex Tkachman <alex.t...@gmail.com> wrote:

Main difference with mine proposal is ability to compile several
versions of the same code and make them available for end user.


To get two compiles out of one code base you could create two versions of MyConstants.java, one with DEBUG = true, one with DEBUG = false . Place one version on the project source path. Place the other is an alternate directory. Then modify your MyProject-compile script to compile twice:
  1. In the normal way using just the project source.
  2. Again (to a different output directory) with the alternate MyConstants.java top level directory included in the classpath before the rest of the project source.
Now that you have two version of the compiled application, you could host each on a separate URL. Maybe still not quite what you were looking for...

Fred





Scott Blum

unread,
Feb 12, 2007, 7:13:30 PM2/12/07
to Google-Web-Tool...@googlegroups.com
This idea is not nearly as practical or elegant as the original
proposal. Alex is suggesting you could simply declare a property in
your module xml with legal values of true and false, write a snippet
of JS to provide the correct value, and then reference the property
directly in code via GWT.booleanProperty("debug") which would be a
guaranteed constant. Then your desired permutations are automatically
built. "debug" would be required to be a string literal /
compile-time constant.

Open question: what happens if the property is not defined? Is that a
compile time error, or is a default value supplied? I could imagine
writing some reusable code that would take advantage of a "debug"
property if it was defined and true, but not require the user to
define it.

On 2/12/07, Fred Sauer <fr...@allen-sauer.com> wrote:

Fred Sauer

unread,
Feb 12, 2007, 10:32:30 PM2/12/07
to Google-Web-Tool...@googlegroups.com
On 2/11/07, Alex Tkachman <alex.t...@gmail.com> wrote:
What I would love to be able to write is

if ( GWT.getBooleanProperty("debug",false) )
{
  // debug code
}

Maybe there's a nice way the properties can be scoped to a specific gwt module, so a "debug" property in an inherited .gwt.xml file does not get clobbered by "debug" in another module.

If your module is "com.mycompany.MyApplication" and your .gwt.xml file has a "debug" property, you could write:

if ( GWT.getBooleanProperty("com.mycompany.MyApplication.debug",false) )
{
  // debug code
}



mP

unread,
Feb 12, 2007, 10:36:35 PM2/12/07
to Google Web Toolkit Contributors
Whats wrong with using a class with some Constants ?

Scott Blum

unread,
Feb 12, 2007, 10:40:38 PM2/12/07
to Google-Web-Tool...@googlegroups.com
Nothing.

Sandy McArthur

unread,
Feb 12, 2007, 11:36:54 PM2/12/07
to Google-Web-Tool...@googlegroups.com
On 2/12/07, Scott Blum <sco...@google.com> wrote:
> Alex is suggesting you could simply declare a property in
> your module xml with legal values of true and false, write a snippet
> of JS to provide the correct value, and then reference the property
> directly in code via GWT.booleanProperty("debug") which would be a
> guaranteed constant. Then your desired permutations are automatically
> built. "debug" would be required to be a string literal /
> compile-time constant.

So, this is just like the existing functionality but you don't have to
declare a class and use replace-with. That saves a little developer
effort but not much.

> Open question: what happens if the property is not defined? Is that a
> compile time error, or is a default value supplied? I could imagine
> writing some reusable code that would take advantage of a "debug"
> property if it was defined and true, but not require the user to
> define it.

Personally, I think of hosted mode as my debug mode and web mode as
non-debug mode. In that situation I've found GWT.isScript() to be a
sufficient answer to whether or not my code is running in "debug"
mode.

Also, all the examples are about a "debug" property. What other
properties would anyone want?
The only property I can think of that isn't a one-off is for the
current browser. I think this feature would encourage the use of messy
code like:

if ("ie6".equals(GWT.getProperty("user.agent"))) {
// IE widget logic
} else if ("gecko".equals(GWT.getProperty("user.agent"))) {
// Gecko widget logic
} else if ....

Seems to me adding support for enabling assert in web mode is a better
course of action without a compelling list of frequently used
properties beyond "debug".

Scott Blum

unread,
Feb 13, 2007, 1:55:26 AM2/13/07
to Google-Web-Tool...@googlegroups.com
On 2/12/07, Sandy McArthur <sand...@gmail.com> wrote:
> So, this is just like the existing functionality but you don't have to
> declare a class and use replace-with. That saves a little developer
> effort but not much.

I think that's a good summary.

> Also, all the examples are about a "debug" property. What other
> properties would anyone want?

Good question. I could see logging as a separate thing from debug. I
know there's GWT.log, but you might want to do custom loggin in web
mode. We could also use it to toggle certain runtime checks -- should
iterators check for concurrent modification? Should ArrayList range
check indices?

> The only property I can think of that isn't a one-off is for the
> current browser. I think this feature would encourage the use of messy
> code like:
>
> if ("ie6".equals(GWT.getProperty("user.agent"))) {
> // IE widget logic
> } else if ("gecko".equals(GWT.getProperty("user.agent"))) {
> // Gecko widget logic
> } else if ....

Granted. (Although the compiler wouldn't actually optimize this out today.)

> Seems to me adding support for enabling assert in web mode is a better
> course of action without a compelling list of frequently used
> properties beyond "debug".

Oh, I absolutely agree that enabling assertions in web mode is
something that needs to happen regardless of this. Wonder if there's
time to sneak it into 1.4.... :)

Scott

Alex Tkachman

unread,
Feb 13, 2007, 4:38:33 AM2/13/07
to Google Web Toolkit Contributors
Patch I've provide yesterday contains code elimination for

"gecko".equals(GWT.getProperty("user.agent"))

I also have working code for asserts, which hope to provide today
(maybe even together with stack traces)

On Feb 13, 9:55 am, "Scott Blum" <sco...@google.com> wrote:

John Tamplin

unread,
Feb 13, 2007, 7:57:42 AM2/13/07
to Google-Web-Tool...@googlegroups.com
On 2/13/07, Scott Blum <sco...@google.com> wrote:
Good question.  I could see logging as a separate thing from debug.  I
know there's GWT.log, but you might want to do custom loggin in web
mode.  We could also use it to toggle certain runtime checks -- should
iterators check for concurrent modification?  Should ArrayList range
check indices?

Wouldn't that be better done using deferred binding to switch in a different implementation class that does the extra checks?

--
John A. Tamplin
Software Engineer, Google

Alex Tkachman

unread,
Feb 13, 2007, 8:02:35 AM2/13/07
to Google Web Toolkit Contributors
Attached file contains preliminary patch for 4 different but related tasks:

1) WebMode stack traces
2) Asserts
3) Conditional compilation
4) Dead code elimination (or better say compile-time calculation) for
constant calls to methods of String class.

Probably only last one can be separated to another patch but I use
conditional compilation for implementation of asserts and stacktraces
and conditional compilation itself requires String-precalculation.

I would really appreciate if somebody familiar with GWT compiler
provide feedback and critique.

Alex

Compile_time_properties+Assert+StackTrace.patch

Scott Blum

unread,
Feb 13, 2007, 10:07:58 AM2/13/07
to Google-Web-Tool...@googlegroups.com

It depends. In general, you can't use deferred binding with a class
that's supposed to be directly instantiated (like ArrayList directly);
you'd have to instantiate a private impl class via deferred binding;
but then you get into performance issues. You'd think that the
iterator class could be defbound because it's instantiated by the
outer class; however the problem here is that you can't use deferred
binding to produce a non-static inner class, because how are you going
to hook up the outer ref? (Tangent: wonder if it'd be possible to
have a GWT.create(Class, Object outer) to allow just that?)

Scott

Alex Tkachman

unread,
Feb 13, 2007, 4:08:14 PM2/13/07
to Google Web Toolkit Contributors
Better patch for the same content.

But do I have any hope for feedback?

Compile_time_properties.patch

Scott Blum

unread,
Feb 13, 2007, 4:32:44 PM2/13/07
to Google-Web-Tool...@googlegroups.com
On 2/13/07, Alex Tkachman <alex.t...@gmail.com> wrote:
> Better patch for the same content.
>
> But do I have any hope for feedback?

So... the thing is that we're trying to push for a 1.4 RC1 next
Wednesday, and I have more things on my plate for this RC than I
actually have time for. :) All of which means that, I probably won't
have time to do an in-depth look at things that aren't slated for 1.4.
I don't think conditional compilation is a candidate, because it's an
API change that would need to be vetted by a wider set of people.
Asserts would be a consideration, except that being implemented in
terrms of cond-comp kind of rules it out. That leaves stack traces
and dead code on Strings, which I hope to have time to look at.

The other big things on my plate are:

- Optionally output JS files directly (rather than HTML); waiting on
jgw's startup optimizations
- Refactor JS naming strategy (inspired by half of issue #610, the
name-allocation patch which proves how much low-hanging fruit there
is)
- The other half of issue #610, merging string constants
- Get patch for issue #622 reviewed & submitted

Scott

Sandy McArthur

unread,
Feb 13, 2007, 5:33:09 PM2/13/07
to Google-Web-Tool...@googlegroups.com
On 2/13/07, Alex Tkachman <alex.t...@gmail.com> wrote:
> But do I have any hope for feedback?

I've read your patch in a text editor but haven't applied it and built
it. Below are my thoughts as I cruised down the file.

* There is a lot of reformatting noise. Changes that just introduce
white space make it harder to read.

* Your mixing too many changes in one patch. I know some of the parts
build on the others but things like the string literal method call
optimization have a chance of being accepted more easily if it was
split out. Right now the chance of a GWT dev taking the time to
extract it from your patch is low. I don't know about the GWT project,
but at Apache we won't accept an overloaded patch. Considering GWT
borrows a lot of policies from Apache and they try to keep discussions
to one topic per thread I suspect they feel the same way.

* Is the string literal method call optimization a really a dead code
elimination or a type of inlining?

* Scott has stated in the past he prefers the StackTrace method that
fills in the stack trace as an exception bubbles out. This means stack
traces will be incomplete and only contain the top few frames but it
will have very low performance overhead in the non-exceptional case.
Also, I'm not sure if the bubble up stack fill method will be a
compile time option or an always there feature once added. Probably
depends on how it affects generated code size and performance.

* removing a blank line and then adding another blank line should be
avoided. If your IDE keeps putting those in the patch, I'll manually
delete them from the patch.

* You widen the scope of the JStringLiteral class when you don't need
to. JProgram has a getLiteralString(char[]) method to create
JStringLiterals and if you look at it's source you'll it's part of
future plans for string literal interning. If you don't like that it
takes a char[] param you should add a getLiteralString(String)
variant.

* again with the unneeded formating changes.

* I suspect the addition of setElseStmt and setThenStmt to
JIfStatement aren't gonna fly. I think you either need to use the
Context to mutate them or create a new JIfStatement instance to
replace an existing one with Context. Same for setBody in
JForStatement and JWhileStatement.

Scott Blum

unread,
Feb 13, 2007, 6:14:24 PM2/13/07
to Google-Web-Tool...@googlegroups.com
Not having looked at the patch yet myself, everything Sandy wrote sounds right.

> * Scott has stated in the past he prefers the StackTrace method that
> fills in the stack trace as an exception bubbles out. This means stack
> traces will be incomplete and only contain the top few frames but it
> will have very low performance overhead in the non-exceptional case.
> Also, I'm not sure if the bubble up stack fill method will be a
> compile time option or an always there feature once added. Probably
> depends on how it affects generated code size and performance.

Just want to be clear that I'm not committed to an approach. I
*think* I'm leaning towards what you describe, but I really have to
look into it in a lot more detail. It may be that some way of
preserving the whole trace is not out of the question. Either way, I
feel quite confident that stack traces will be a toggle option that
will be off by default.

Scott

Alex Tkachman

unread,
Feb 13, 2007, 8:55:47 PM2/13/07
to Google-Web-Tool...@googlegroups.com
I got all Sandy points except last one.
Could you explain what's wrong with in-place replacement of for-body etc.?

I also got your point about huge amount of tasks before 1.4RC But let
me do last try. My problem is I expect to be extremely busy several
weeks after this one and will not be able to participate here
actively.

I cut huge patch to several smaller ones. They cover asserts, stack
traces and compile time calculation of calls to methods of String
class with literal parameters. Conditional compilation is out of scope
before we agreed on API. I believe such cut significantly simplifies
review. And of course I think functionality itself (at least asserts
and stack traces) is important enough to try to include it in to 1.4.

See attached files.

Make_some_Java_AST_statements_mutable.patch
Debug_info_infrastructure_-_User_code.patch
Debug_info_infrastructure_-_Compiler_code.patch
Asserts_&_StackTrace.patch
Constant_string_operations.patch

Scott Blum

unread,
Feb 20, 2007, 6:45:19 PM2/20/07
to Alex Tkachman, Google-Web-Tool...@googlegroups.com
Hi Alex,

Something is weird about these patches, there are no "Index:" lines.

For example, a recent patch I made begins:

Index: dev/core/src/com/google/gwt/dev/jdt/FindJsniRefVisitor.java
===================================================================
--- dev/core/src/com/google/gwt/dev/jdt/FindJsniRefVisitor.java    (revision 429)
+++ dev/core/src/com/google/gwt/dev/jdt/FindJsniRefVisitor.java    (working copy)

Yours just starts in:
--- trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JDoStatement.java    (revision 396)
+++ trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JDoStatement.java    Wed Feb 14 03:43:06 MSK 2007

Are you using an old version of svn or something?  The reason I ask is that the tool I'm using won't handle your patch files unless I hand-edit them.

Thanks,
Scott


On 2/13/07, Alex Tkachman <alex.t...@gmail.com> wrote:

Scott Blum

unread,
Feb 21, 2007, 8:47:30 PM2/21/07
to Google-Web-Tool...@googlegroups.com
Hi all,

I've had a chance to take a look at this now, so let me go through point by point.

1) Making Java AST statements mutable

We actually just got this out of the system, and I'd rather not put it back in.  The change you really want is actually far more fundamental-- you want normalization of the Java AST tree.  Every type of statement has a JStatement as a child today should probably have a JBlock instead; this would allow much nicer optimizing and avoid the weird cases of having to either remove yourself or replace yourself with an empty block.  We would have to optimize either going from Java->JS or generating the JS itself to suppress unnecessary extra braces, but we should be doing this anyway if we're not.  I think this merits its own discussion, because it would totally solve what you want.

2) Debug info infrastructure.

I think we want to do something like this, just not for 1.4.  I also think we need a larger discussion of whether gwt.debug is how we want to do it, and the specific formulation.  The way it is presently, we'd end up doubling the number of permutations by default, for every compile.  I think we'd rather have debug off by default unless someone specifically turns it on.  Again, this seems like its own design discussion.

3) Asserts

I really want to do this, but we can't do it without settling on how to turn debugging on and off, so it won't be able to go into 1.4. 

Technical critique:
- I think we have some correctness issues-- you always call Exceptions.doAssert() which takes a String as the second argument, but what you are passing in might not be a String.  We should be calling the correctly-typed constructor.
- Instead of allowing the arg to be null, it would be better to synthesize the String literal during GenerateJavaAST.  This would also allow us to grab the real source text directly from JDT's AST.

4) Stack trace

Again, I'm still a fan of catching the exception at every level and fill in the stack trace as you go with local info, resulting in almost no overhead when no exception is thrown.  I suppose I could be convinced that the extra stack frames you get with your suggested approach might be worth the extra overhead, since it is a debug build.

Some critiques, even given this approach:
- We definitely don't want to be doing an array.push() / array.pop() at every level.  It's much faster to just keep an index handy and don't bother cleaning up.  The memory is insignificant, and some browsers (IE) take O(n^2) to push/pop, as we learned the hard way in RPC.
- Alternatively, since we're creating our own list of stack trace elements that are totally an implementation detail, why not just add an extra field and link-list them?
- Either way, the current "top" element should be readily available to make line number updates fast.
- It doesn't handle JSNI code.  Granted, this is an extremely minor point (since hosted mode won't do that either), but we could actually make stack traces work for JSNI methods.

If I were to take these ideas to the logical extreme, I would probably want to do this at the JS level (after some additional infrastructure work to get good source info into the JS AST).  I would reserve a single well-known global identifier for the stack trace, which would be a linked list.  A method might look like this (sans obfuscation) :

function foo() {
  try {
    $globalException = {file:"Blah.java",line:27,className:"com.example.Blah",methodName:"foo",next:$globalException};
    // stmt 1
    $globalException.line = 29;
    // stmt 2
    ++$globalException.line; // :)
  } catch (e) {
    fillInStackTrace(e);
  } finally {
    $globalException = $globalException.next;
  }
}

5) Constant string operations

This is really cool!  I think I would like to try to get this into 1.4.  Overall it looks really good, although we should maybe factor out some of the guts of it.  It would be good if we also handled the constructors and public static methods.

Scott

On 2/13/07, Alex Tkachman <alex.t...@gmail.com> wrote:

Scott Blum

unread,
Feb 21, 2007, 8:51:08 PM2/21/07
to Google-Web-Tool...@googlegroups.com
On 2/21/07, Scott Blum <sco...@google.com> wrote:
3) Asserts

I really want to do this, but we can't do it without settling on how to turn debugging on and off, so it won't be able to go into 1.4. 

Technical critique:
- I think we have some correctness issues-- you always call Exceptions.doAssert() which takes a String as the second argument, but what you are passing in might not be a String.  We should be calling the correctly-typed constructor.
- Instead of allowing the arg to be null, it would be better to synthesize the String literal during GenerateJavaAST.  This would also allow us to grab the real source text directly from JDT's AST.

Couple of things I forgot:
1) I want to go ahead and put your AssertNormalizer in, but have it just always dead-strip for now.  The fact that we're currently not pulling it out until GenerateJavaScriptAST could be blocking other optimizations.

2) Speaking of GenerateJavaScriptAST, we need to throw new InternalCompilerException("Should not get here."); if an AssertStatement makes it that far.

Scott

Alex Tkachman

unread,
Feb 22, 2007, 1:41:25 AM2/22/07
to Google-Web-Tool...@googlegroups.com
Scott!

Thank you for taking time looking on that. I seems like you checked
not the last version, which attached to comment in issue 692. Here is
a link for this comment posted 6 days ago
http://code.google.com/p/google-web-toolkit/issues/detail?id=692&can=2&q=alex.tkachman#c5

BTW, I completely agree with Dan Morrill, who marked this issue this
Priority-High for 1.4RC. And let me repeat my arguments from another
post on the same issue "it is absolutely unbelievable feeling to catch
in your IDE an exception raised by assert statement in some test
running in web mode... For me it is exactly one of the things, which
is all GWT about"

Let me try to comment point by point now.

> 1) Making Java AST statements mutable
>
> We actually just got this out of the system, and I'd rather not put it back
> in. The change you really want is actually far more fundamental-- you want
> normalization of the Java AST tree. Every type of statement has a
> JStatement as a child today should probably have a JBlock instead; this
> would allow much nicer optimizing and avoid the weird cases of having to
> either remove yourself or replace yourself with an empty block. We would
> have to optimize either going from Java->JS or generating the JS itself to
> suppress unnecessary extra braces, but we should be doing this anyway if
> we're not. I think this merits its own discussion, because it would totally
> solve what you want.
>
> 2) Debug info infrastructure.
>
> I think we want to do something like this, just not for 1.4. I also think
> we need a larger discussion of whether gwt.debug is how we want to do it,
> and the specific formulation. The way it is presently, we'd end up doubling
> the number of permutations by default, for every compile. I think we'd
> rather have debug off by default unless someone specifically turns it on.
> Again, this seems like its own design discussion.
>

In my last patch I've removed doubling of permutation number. All you
need to do is put <set-property name="gwt.debug" value="true"/> in one
of your modules. But I would suggest even better solution - compiler
swtith "-debug", which will force this property on.

> 3) Asserts


> Technical critique:
> - I think we have some correctness issues-- you always call
> Exceptions.doAssert() which takes a String as the second argument, but what
> you are passing in might not be a String. We should be calling the
> correctly-typed constructor.

Good point.

> - Instead of allowing the arg to be null, it would be better to synthesize
> the String literal during GenerateJavaAST. This would also allow us to grab
> the real source text directly from JDT's AST.
>

My last patch does it already.And it is really very convenient.

> 4) Stack trace


>
> Some critiques, even given this approach:
> - We definitely don't want to be doing an array.push() / array.pop() at
> every level. It's much faster to just keep an index handy and don't bother
> cleaning up. The memory is insignificant, and some browsers (IE) take
> O(n^2) to push/pop, as we learned the hard way in RPC.

God, I never new about that. The only reason for me to use some many
native code was wrong hope that array.push/pop are extremely fast.

> - Alternatively, since we're creating our own list of stack trace elements
> that are totally an implementation detail, why not just add an extra field
> and link-list them?
> - Either way, the current "top" element should be readily available to make
> line number updates fast.

Of course, we should do that. It is easy and I will be happy to do
that. Fortunately I have tests :)

> - It doesn't handle JSNI code. Granted, this is an extremely minor point
> (since hosted mode won't do that either), but we could actually make stack
> traces work for JSNI methods.
>

My last patch does that already. File JavaScript_source_info.patch
contains necessary modification for work with JSNI code.

> If I were to take these ideas to the logical extreme, I would probably want
> to do this at the JS level (after some additional infrastructure work to get
> good source info into the JS AST). I would reserve a single well-known
> global identifier for the stack trace, which would be a linked list. A
> method might look like this (sans obfuscation) :
>
> function foo() {
> try {
> $globalException =
> {file:"Blah.java",line:27,className:"com.example.Blah",methodName:"foo",next:$globalException};
> // stmt 1
> $globalException.line = 29;
> // stmt 2
> ++$globalException.line; // :)
> } catch (e) {
> fillInStackTrace(e);
> } finally {
> $globalException = $globalException.next;
> }
> }
>

Scott, I agree that it would be great. But it requires some
infrastructural changes in compiler.

Let us look on that from customer (oh, sorry user :) ) point of view -
we are either able to provide really great functionality, which
dramatically change development experience immediately, or wait for
at least couple of months before we are ready to implement it most
elegant way. Probably I am staying here on businessman position not on
engineer one but my choice (well, suggestion) is to improve the patch
deliver in 1.4.

Bruce Johnson

unread,
Feb 22, 2007, 9:10:43 AM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, Alex Tkachman <alex.t...@gmail.com> wrote:
> BTW, I completely agree with Dan Morrill, who marked this issue this
> Priority-High for 1.4RC.

I don't doubt the coolness of the feature, but I think Dan's making
this part of 1.4 must have been an accident.

We must freeze new features for 1.4 ASAP. I am planning to remove the
1.4 Milestone label for this issue, but it sounds like you guys are
getting so close to a working solution that it should be pretty easy
to slip into the subsequent release.

I've mostly been just lurking on this thread, but I'll chime in just
to say that the reason it seems like the absence of web-mode stack
traces is not urgently important is that if GWT works the way it's
supposed to, it should never be useful in practice. In other words,
any problem that you would find in web mode should already have been
found in hosted mode, where you have an arsenal of good tools. If GWT
users are having problems that can only be diagnosed in web mode, then
something more fundamental needs to be addressed first.

-- Bruce

John Tamplin

unread,
Feb 22, 2007, 9:25:38 AM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, Bruce Johnson <br...@google.com> wrote:

On 2/22/07, Alex Tkachman <alex.t...@gmail.com> wrote:
> BTW, I completely agree with Dan Morrill, who marked this issue this
> Priority-High for 1.4RC .

I don't doubt the coolness of the feature, but I think Dan's making
this part of 1.4 must have been an accident.

Dan said he was marking things Critical and High to ensure that someone in the area looked at it at made a decision on it, with Critical including either showstopper bugs or trivial fixes, and anything below High wasn't likely to go in unless it was picked up as part of something else or we had time left over.

For example, I haven't gone through my bugs as I have been busy on Linux hosted mode, so I am sure things are still marked as High that aren't going into 1.4RC1 either.

Bruce Johnson

unread,
Feb 22, 2007, 11:03:24 AM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, John Tamplin <j...@google.com> wrote:
On 2/22/07, Bruce Johnson <br...@google.com> wrote:
Dan said he was marking things Critical and High to ensure that someone in the area looked at it at made a decision on it, with Critical including either showstopper bugs or trivial fixes, and anything below High wasn't likely to go in unless it was picked up as part of something else or we had time left over.

I am aware of that. He marked it as being part of "Milestone 1.4", which is the part that I object to.
 

Alex Tkachman

unread,
Feb 22, 2007, 11:12:11 AM2/22/07
to Google-Web-Tool...@googlegroups.com
Bruce,

I understand your reasons about the schedule. Let us postpone till
next releases.

But I don't agree with you about importance of web mode stack traces.
Hosted mode is great solution for developer, who usually works on one
platform, but tests should be run on all possible combination of
platforms and browsers and stacktraces could be absolutely unique tool
here. And it is especially criticalfor external javascript.
Fortunately sooner or later we plan (as I understand) to involve it in
to our compilation process.

Regarding testing iteself I believe we have to think again about
better infrastructure for remote run of tests.

Alex

On 2/22/07, Bruce Johnson <br...@google.com> wrote:
>

Sandy McArthur

unread,
Feb 22, 2007, 11:31:19 AM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, Bruce Johnson <br...@google.com> wrote:
> I've mostly been just lurking on this thread, but I'll chime in just
> to say that the reason it seems like the absence of web-mode stack
> traces is not urgently important is that if GWT works the way it's
> supposed to, it should never be useful in practice. In other words,
> any problem that you would find in web mode should already have been
> found in hosted mode, where you have an arsenal of good tools. If GWT
> users are having problems that can only be diagnosed in web mode, then
> something more fundamental needs to be addressed first.

I'm with mostly Alex on this one. It's unreasonable for hosted mode to
support every combination of browser extensions or popup blockers and
simulate various firewalls or unreliable internet connections. A
method for users to be able to send in partial stack traces would help
a lot when trying to nail a user's problem and you cannot travel to
their home and experience the problem yourself.

I'll agree they aren't urgent, I'm surviving without them today. I
just think hosted mode isn't a substitute for them.

Scott Blum

unread,
Feb 22, 2007, 11:46:28 AM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, Alex Tkachman <alex.t...@gmail.com> wrote:
Thank you for taking time looking on that. I seems like you checked
not the last version, which attached to comment in issue 692. Here is
a link for this comment posted 6 days ago
http://code.google.com/p/google-web-toolkit/issues/detail?id=692&can=2&q=alex.tkachman#c5

Argh, I *thought* I remembered you putting in an issue for this, but I swear when I went looking for it, I couldn't find it, so I just used what was on this thread.  I'll take another look, but as Bruce mentioned, we're really trying to nail down 1.4 so there's very little chance it could go into this release.

In my last patch I've removed doubling of permutation number. All you
need to do is put <set-property name=" gwt.debug" value="true"/> in one
of your modules. But I would suggest even better solution - compiler
swtith "-debug", which will force this property on.

Yeah, I'm almost in favor of a compile flag myself, but there's a bit of a debate about compiler flags vs. module properties. :)


> - Instead of allowing the arg to be null, it would be better to synthesize
> the String literal during GenerateJavaAST.  This would also allow us to grab
> the real source text directly from JDT's AST.
>

My last patch does it already.And it is really very convenient.

Sweet!

> - It doesn't handle JSNI code.  Granted, this is an extremely minor point
> (since hosted mode won't do that either), but we could actually make stack
> traces work for JSNI methods.

My last patch does that already. File JavaScript_source_info.patch
contains necessary modification for work with JSNI code.

Double sweet! 

Fair enough.  But if it can't make 1.4, then we should make whatever changes we need to do it the best way possible.

Scott

Sandy McArthur

unread,
Feb 22, 2007, 12:32:25 PM2/22/07
to Google-Web-Tool...@googlegroups.com
On 2/22/07, Scott Blum <sco...@google.com> wrote:
> Yeah, I'm almost in favor of a compile flag myself, but there's a bit of a
> debate about compiler flags vs. module properties. :)

Where is this debate? I'd like to throw my $0.02 towards everything
being a property so that if I wanted every permutation of -style, -ea,
debug, user.agent, etc generated and selectable at load time I could
have them.

Reply all
Reply to author
Forward
0 new messages