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 { just to make sure I understand - looks like you replaced this with either the name of the class literal (if passed) or getImplicitName() in ReplaceRunAsyncs. 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; Dumb question. What does this JDT MessageSend class represent? It looks to me like we would want to look for static method invocations if we are looking for GWT.create() calls. I see GWT.create() has a complex declaration using generics, does that make it special? public static <T> T create(Class<?> classLiteral) 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); On 2011/05/20 22:47:15, scottb wrote:
This was an indexed type anyway.
just to clarify, What you are saying is that it would have been found in the reachability analysis without explicitly adding it as a root type. Now you are adding a direct call to AsyncFragmentLoader.runAsync() via visiting a JAsyncNode CloneExpressionVisitor returned from JRunAsync.getRunAsyncCall(). 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()) { Thinking out loud... is this still going to work? We won't replace GWT.runAsync() calls with JRunAsyncNodes so CloneExpressionVisitor won't add references to AsyncFragmentLoader.runAsync(). So what will happen to them instead? nm, I see the standard implementation of GWT.runAsync() is GWT.runAsyncWithoutCodeSplitting() 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() { This is nice; it seems a lot clearer than before the patch. 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: /** nice that you don't need this anymore. 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("@")) { I'm surprised to find all this JSNI checking here (What about JSNI checker?). why would this be any different in the split point logic than a monolithic program? Are these just assertions to make sure liveness checks are working correctly? 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(); Aside: all this referencing to split points by number and keeping track of when to add or subtract 1 make me think - could we define some kind of SplitPoint class instead or is there a good reason to keep them as numbers? (but not in this patch, please.) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java File dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode373 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:373: */ makes sense... 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)) { can you tell me why you decided this was not necessary? (not that I understand why it was there initially...) 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: /* just to be sure I understand, this code that generated JS is replaced by translatable GWT code in AsyncFragmentLoader.java... 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; On 2011/05/20 22:47:15, scottb wrote:
Some fragments are now so small, they don't contain any functions. :)
Did this prevent a bug? There's likely no sense in sorting 3, 4, 5 or even 25 methods either. I'd guess that only fragments over a certain size benefit from clustering methods together. OK, this is completely unrelated to FunctionClusterer, but it brings to mind an observation: I've noticed some developers creating very small fragments with runAsync(). From a performance standpoint, a 2k split point in an >1M app is unlikely to be an overall win. I wonder if it would make sense to add a threshold below which we'll just throw it into the leftovers fragment? or join fragments together into a joined fragment? Potentially users could use runAsync() more liberally and not have an explosion of itty bitty downloadable artifacts. 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) { Was this made unecessary by the explicit rescue in ControlFlowAnalyzer.traverseFrom()? 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); On 2011/05/20 22:47:15, scottb wrote:
This was just doing a more work than it needed to.
So, none of the changes in this file are related to the rest of the patch? Could you submit it separately, just in case we have to roll back? 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 Looks like some folks specify this with the files filter up above and then some use the packages list to do the same thing. Isn't "users keep out" that what the impl pacakge means? If so, why not specify com.google.gwt.**.impl/** in packages, or if not, just use the files exclusion up above? http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/GWT.java File user/src/com/google/gwt/core/client/GWT.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/GWT.java#newcode248 user/src/com/google/gwt/core/client/GWT.java:248: callback.onSuccess(); On 2011/05/20 22:47:15, scottb wrote:
What I have done here is make it so that the code simply behaves as if
the
target fragment were already loaded. I don't think the stats events
(that don't
match real splitting anyway) are at all important. And there's no
need to trap
exceptions with the UEH either, if the caller doesn't catch the
exception, the
UEH will get it eventually anyway.
Thanks for the explanation. I agree, I don't think the 'noDownloadNeeded' stats events are useful. I'm not sure why the UEH code would have ever been necessary, either. http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java File user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode583 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:583: } On 2011/05/20 22:47:15, scottb wrote:
Essentially, the above code used to be generated into each fragment
loader. I much prefer this as straight Java code... 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()) { !?! I assume this is an unrelated bug fix. Has anyone complained about this? Add a unit test? (no good deed goes unpunished) http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
