[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
On Tue, Feb 10, 2009 at 7:38 PM, Ray Cromwell wrote: > Bob, you are awesome. I'm excited this went from design to > implementation so quickly, since it solves a lot of headache in my > current codebase. I only wish it made it into 1.6. :) s/Bob/GWT team/ Thanks Ray. I'm sure you'll have a highly-informative blog post about it in no time. ;-) -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
Bob, you are awesome. I'm excited this went from design to implementation so quickly, since it solves a lot of headache in my current codebase. I only wish it made it into 1.6. :) -Ray On Tue, Feb 10, 2009 at 4:31 PM, BobV wrote: > > I have committed the SingleJsoImpl branch to trunk at r4689. > > Further comments from Lex's review: > - Fixed JSORestrictionsChecker to actually implement what's in the design > doc. > - Updated jsoRestrictions.html and changed JavaDoc in JSORC to point > to the design doc so that there's one canonical source of what > _should_ be implemented. > - Removed the allowJso() flag from JCastOperation and JInstanceOf in > favor of making CastNormalizer check against the list dual-impl > interfaces > - Renamed JTypeOracle.recomputeClinit() to > JTypeOracle.recomputeAfterOptimization() because it also causes > JsoSingleImpl data to be recomputed. This ensures that pruning > interacts favorably with the choice of methods used by CastNormalizer. > > -- > Bob Vawter > Google Web Toolkit Team > > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
I have committed the SingleJsoImpl branch to trunk at r4689. Further comments from Lex's review: - Fixed JSORestrictionsChecker to actually implement what's in the design doc. - Updated jsoRestrictions.html and changed JavaDoc in JSORC to point to the design doc so that there's one canonical source of what _should_ be implemented. - Removed the allowJso() flag from JCastOperation and JInstanceOf in favor of making CastNormalizer check against the list dual-impl interfaces - Renamed JTypeOracle.recomputeClinit() to JTypeOracle.recomputeAfterOptimization() because it also causes JsoSingleImpl data to be recomputed. This ensures that pruning interacts favorably with the choice of methods used by CastNormalizer. -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
Let's also be sure that the appropriate optimizers can statically resolve type checks on SingleJsoImpl interfaces. On Thu, Jan 15, 2009 at 1:02 PM, BobV wrote: > > That's the flag I asked about. In general it makes perfect sense to > > update the AST to hold any information that is needed. In this case, > > though, if I understand correctly, the information is soley determined > > by the target type of the cast. > > I put the flag in there so that JsoNormalizer would contain all of the > AST fix-up code relevant to the JSO type instead of splitting it > across visitors. I'm fine with getting rid of the flag entirely and > moving the flag-setting code from JsoNormalizer into CastNormalizer. --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
> That's the flag I asked about. In general it makes perfect sense to > update the AST to hold any information that is needed. In this case, > though, if I understand correctly, the information is soley determined > by the target type of the cast. I put the flag in there so that JsoNormalizer would contain all of the AST fix-up code relevant to the JSO type instead of splitting it across visitors. I'm fine with getting rid of the flag entirely and moving the flag-setting code from JsoNormalizer into CastNormalizer. -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
On Thu, Jan 15, 2009 at 12:01 PM, Scott Blum wrote: > This sounds great. My only comment involves this statement: " > > Casts and instanceof checks are accomplished by adding a flag > to JCastOperation and JInstanceOf to allow a non-null, non-Java-derived > (i.e.o.typeMarker$ != nullMethod) object to pass the type checks. Additional > methods are added to the Cast utility class and CastNormalizerupdated." I > believe based on reading this, but wanted to verify, that the 'flag' exists > only in the compiler (not the compiled code) and different exact variants of > dynamicCast/instanceOf are called at runtime. That's the flag I asked about. In general it makes perfect sense to update the AST to hold any information that is needed. In this case, though, if I understand correctly, the information is soley determined by the target type of the cast. Mainly I worry that any code constructing a cast operation will need to be sure and set allowJso correctly, won't it? How does the code know what to supply? As a concrete example, there is an upcoming patch to add bridge methods in GenerateJavaAST. The bridge methods will need cast operations. How does the bridge-adder know what to set allowJso to, and should the bridge-adder have to worry about JSO's at all, anyway? -Lex --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
On Thu, Jan 15, 2009 at 12:01 PM, Scott Blum wrote: > Casts and instanceof checks are accomplished by adding a flag > to JCastOperation and JInstanceOf to allow a non-null, non-Java-derived > (i.e.o.typeMarker$ != nullMethod) object to pass the type checks. Additional > methods are added to the Cast utility class and CastNormalizerupdated." I > believe based on reading this, but wanted to verify, that the 'flag' exists > only in the compiler (not the compiled code) and different exact variants of > dynamicCast/instanceOf are called at runtime. @Lex, thanks for the review, I'll add the changes to the branch, probably on Monday or thereabouts. @Scott, Yes, the flag is compiler-only, it's an indicator to CastNormalizer as to which Cast method invocation the JCastOperation should be replaced with. -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
This sounds great. My only comment involves this statement: " Casts and instanceof checks are accomplished by adding a flag to JCastOperation and JInstanceOf to allow a non-null, non-Java-derived (i.e.o.typeMarker$ != nullMethod) object to pass the type checks. Additional methods are added to the Cast utility class and CastNormalizerupdated." I believe based on reading this, but wanted to verify, that the 'flag' exists only in the compiler (not the compiled code) and different exact variants of dynamicCast/instanceOf are called at runtime. On Wed, Jan 14, 2009 at 2:31 PM, BobV wrote: > On Tue, Jan 13, 2009 at 10:49 AM, BobV wrote: > > On Tue, Jan 13, 2009 at 10:41 AM, Scott Blum wrote: > >> Sure, is there a design doc for this so that I know what I'm looking at? > > I've added a section at the bottom of > http://code.google.com/p/google-web-toolkit/wiki/OverlayTypes that > describes the current work. > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
I have reviewed the JJS parts and the JSORestrictionsChecker part. That leaves hosted mode (dev/shell). I like the new rules that allow Java implementations to also exist, albeit at a performance penalty. I have one question about the new checking rules: is the check at JSORestrictionsChecker lines 125-137 necessary? It actually seems fine to me if an interface extends another interface that extends SingleJsoImpl. This is particularly interesting in the case where the interface actually won't have a JSO implementation, but only Java implementations. With the checking rules in the current patch, such an interface would have to explicitly mark itself as SingleJsoImpl even though it wouldn't be implemented by even one JSO. More generally, marker interfaces would naturally be inherited along with everything else that is inherited, and this doesn't seem like a place to make an exception. The comment of JSORestrictionsChecker still says that no method may override another. This should be updated to reflect the SingleJsoImpl exception. Ideally, the restrictions would all be documented in one place. One way to do that would be to move SingleJsoImpl's comments over to JSORestrictionsChecker. Or better, perhaps they should all be moved to JavaScriptObject, so that they are easily accessible to people using GWT. Did you try making JSORestrictionsChecker and CompilationUnitInvalidator being instantiable? This would remove a few lines of code for defining and passing around the state objects. Does JCastOperation.allowJso simplify things any over simply having CastNormalizer compute the same information? That is, couldn't we move the current test in JSONormalizer into CastNormalizer, and leave JCastOperation as it stands? Likewise for JInstanceOf. If I'm not mistaken, the dualImpl information in JTypeOracle is computed only before optimization. It would be a good TODO in setInstantiatedTypes to say that the info should be recomputed. It's entirely possible that there will be Java implementations of a JSO before optimizations, but that the optimizer will remove the non-Java ones and thus allow a better implementation. That's all. The implementation looks great and the test cases are remarkably thorough. Lex --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
On Tue, Jan 13, 2009 at 10:49 AM, BobV wrote: > On Tue, Jan 13, 2009 at 10:41 AM, Scott Blum wrote: >> Sure, is there a design doc for this so that I know what I'm looking at? I've added a section at the bottom of http://code.google.com/p/google-web-toolkit/wiki/OverlayTypes that describes the current work. -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
On Tue, Jan 13, 2009 at 10:41 AM, Scott Blum wrote: > Sure, is there a design doc for this so that I know what I'm looking at? Not at present, it's pretty much as we discussed. Methods on SingleJsoImpl are renamed so that JSO$ can unambiguously implement them as trampoline dispatches to static methods. Non-JSO types that also implement those interfaces have trampoline methods added to link the mangled name back to the original short name. -- Bob Vawter Google Web Toolkit Team --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: Implementation of SingleJsoImpl
Sure, is there a design doc for this so that I know what I'm looking at? On Mon, Jan 12, 2009 at 9:14 PM, BobV wrote: > The first round of coding is done. > > Follow-up from > http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_thread/thread/5e180695145892d5 > > > @Scott, > Can you review the hosted-mode portions (dev/shell/*)? There's also > a small tweak to GWTRunnerGenerator that traps un-logged exceptions > escaping when test cases are instantiated. > > @Lex, > Can you review the compiler portions (dev/jjs/*)? > > About the only interesting code shared between the compiler and > hosted-mode is JSORestrictionsChecker in dev/javac/*. It's like we're > shipping two products in one! > > svn merge -r4432:HEAD > > https://google-web-toolkit.googlecode.com/svn/changes/bobv/jso_single_impl_r4432 > . > > > -- > Bob Vawter > Google Web Toolkit Team > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
