http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java File dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java (left):
http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java#oldcode131 dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java:131: } else { Exactly, I'm just precomputing essentially the same value and stashing it in the AST node for posterity. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java File dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java#newcode65 dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java:65: String location = runAsync.getName(); On 2011/05/23 14:42:41, bobv wrote:
"location" vs. "getName()". Has the concept changed?
Dunno, the mixed terminology predates my patch. Name is probably more accurate, since it can be either the class literal that 'names' it, or the generated name based on enclosing method (both before and after my patch). http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java File dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java#newcode45 dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java:45: public final MessageSend messageSend; MessageSend is basically any method call, static, instance, generic, whose target is not a constructor. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java#newcode55 dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java:55: public static final String ASYNC_MAGIC_METHOD = "runAsync"; On 2011/05/23 14:42:41, bobv wrote:
Unused?
Done. Whoops. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#oldcode519 dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:519: allRootTypes.add(FragmentLoaderCreator.ASYNC_FRAGMENT_LOADER); Actually, I meant the that immediately preceeding line, "allRootTypes.addAll(JProgram.INDEX_TYPES_SET)" makes the removed line unnecessary, because JProgram.INDEX_TYPES_SET.contains(AsyncFragmentLoader). http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java#newcode617 dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java:617: if (module != null && options.isRunAsyncEnabled()) { Yep. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java#newcode842 dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java:842: public List<JRunAsync> getRunAsyncs() { Thanks, I thought so too. :) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java#newcode1015 dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java:1015: public void setRunAsyncs(List<JRunAsync> runAsyncs) { That is a fantastic observation, and not one I thought about. I believe my patch does not affect the answer to the question, but it seems like a great idea to follow up on. I think what you would want to do is in Pruner.execImpl(), let traverseEntryMethods() tell you which split points are reachable, and only traverse those runAsync points. Of course, the second traversal will reach new points, so you'd have to do it iteratively. Finally, you'd reduce the set of compiler split points to just the reachable set. There would be cleanup work elsewhere with code that assumes that the set is fixed, and that the split point numbers are fixed up front, contiguous, etc. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java File dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java#newcode40 dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java:40: * Based on either explicit class literal, or the jsni name of the containing Technically, it doesn't even need to be unique, but if you have two unnamed splits in the same method, you cannot reference them unambiguously, and attempting to is an error. Actually, I really want to kill the feature that allows you to refer to split points via enclosing method jsni syntax, and force users to name them if they care, but that's beyond the scope of this patch. Just another non-useful feature we added that we have to support. :) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java#oldcode541 dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java:541: /** Few things in life give me as much satisfaction as deleting code. :) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java#newcode271 dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java:271: if (refString.startsWith("@")) { No, it's a mis-feature that allows you to refer to unnamed split points via enclosing method. The referrent isn't even JSNI, it's GWT XML, which JsniChecker never sees. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java#newcode973 dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java:973: int splitPoint = runAsync.getSplitPoint(); Yes, especially with Jason's idea of removing unreachable split points, something like that would be fantastic. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java#oldcode266 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentExtractor.java:266: if (isEntryCall(stat)) { It's not necessary because this code is used to move code from the initial fragment into other fragments, the entry methods all live in the initial fragment and are instrinsically reachable already, I stepped through and verified this code doesn't do anything useful. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java File dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java#oldcode1 dev/core/src/com/google/gwt/dev/jjs/impl/FragmentLoaderCreator.java:1: /* Yes. Basically, the useful parts of the generated code were essentially duplicated for every split point. Now the per-splitcode code is completely trivial, just a call to ASF.onLoad(<number>) and the code is all shared. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java File dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java#newcode1247 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1247: func.setArtificiallyRescued(true); Thought you'd like that. :) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java File dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java#newcode86 dev/core/src/com/google/gwt/dev/jjs/impl/JsFunctionClusterer.java:86: return; It prevented a bug. This optimizer only works on static functions, and I got a fragment with no static functions in it. (This used to be impossible because of the per-fragment generated code.) The fragments aren't completely empty, however, in the case I ran into, it contained vtable setup code for a class or two, including inline polymorphic method setup code, like... _.foo = function foo() { blahblahblah...} http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#oldcode175 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:175: private void tightenCallbackType(int entryNumber, JType callbackType) { That, plus line 97 in the new code, where instead of generating a call to RunAsyncCallback.onSuccess(), we go and find the concrete override and call it directly, so we don't need to invoke MCT to do it for us. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#newcode198 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:198: static String getImplicitName(JMethod method) { Yeah, wasn't sure really where to put this, looked around but didn't find anyone else doing quite this, most similar uses are doing the opposite, breaking a ref down to find what it points to. Could certainly be made more sharable if there are more uses for it, but I actually want this feature to go away. I mean, the feature where exact JSNI name matters as a lookup. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/js/JsUnusedFunctionRemover.java File dev/core/src/com/google/gwt/dev/js/JsUnusedFunctionRemover.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/js/JsUnusedFunctionRemover.java#oldcode118 dev/core/src/com/google/gwt/dev/js/JsUnusedFunctionRemover.java:118: (new JsNameRefVisitor()).accept(program); The change to check f.isArtificallyRescued() instead of name.isObfuscatable() is tied to this patch, actually. I could split that and pieces of GenJSAst into a pre-patch if you think it's worth it. http://gwt-code-reviews.appspot.com/1442807/diff/4001/tools/api-checker/config/gwt22_23userApi.conf File tools/api-checker/config/gwt22_23userApi.conf (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/tools/api-checker/config/gwt22_23userApi.conf#newcode113 tools/api-checker/config/gwt22_23userApi.conf:113: #excluded packages colon separated list It's weird, but I think the stuff above is generally for non-translatable code that the ApiChecker (which is GWT-based) can't analyze at all. Whereas the packages exclusion is for translatable code whose API we don't care about. http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/prefetch/RunAsyncCode.java File user/src/com/google/gwt/core/client/prefetch/RunAsyncCode.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/prefetch/RunAsyncCode.java#newcode60 user/src/com/google/gwt/core/client/prefetch/RunAsyncCode.java:60: if (!GWT.isScript()) { On 2011/05/21 03:28:42, zundel wrote:
!?! I assume this is an unrelated bug fix. Has anyone complained
about this?
Add a unit test? (no good deed goes unpunished)
I couldn't find any callers at all, but Bob agreed it looked like a bug. Feel free to add a unit test in a follow up patch. :) http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
