[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
LGTM, just commenting nits.
http://gwt-code-reviews.appspot.com/1442807/diff/1065/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java
File dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java
(right):
http://gwt-code-reviews.appspot.com/1442807/diff/1065/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java#newcode49
dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java:49: if
(x.isVolatile() || !x.canBePolymorphic()) {
Update comment to account for the x.isVolatile() check?
http://gwt-code-reviews.appspot.com/1442807/diff/6025/dev/core/src/com/google/gwt/dev/jjs/ast/JMethodCall.java
File dev/core/src/com/google/gwt/dev/jjs/ast/JMethodCall.java (right):
http://gwt-code-reviews.appspot.com/1442807/diff/6025/dev/core/src/com/google/gwt/dev/jjs/ast/JMethodCall.java#newcode34
dev/core/src/com/google/gwt/dev/jjs/ast/JMethodCall.java:34: */
thanks for the docs.
http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java#newcode98
dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java:98: * Explcitly
traverse the onSuccessCall.
This comment doesn't really tell us much more than the code itself.
The bug that led to this code was tricky - maybe add a bit more
explanation?
http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode371
dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:371: *
Magic magic magic: don't allow code flow from the AsyncFragmentLoader
(again, not magic...)
http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#newcode66
dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:66: *
Magic magic magic: don't optimizations on the calls from from
don't optimizations == don't run tightening optimizations
I don't think the term 'magic' is appropriate. I think the code below
can be explained through science and reason. For example, you could
just add another sentence or two to explain why tightening optimizations
might defeat code splitting.
http://gwt-code-reviews.appspot.com/1442807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java#newcode98 dev/core/src/com/google/gwt/dev/jjs/ast/JRunAsync.java:98: * Explcitly traverse the onSuccessCall. On 2011/06/02 16:36:44, zundel wrote: This comment doesn't really tell us much more than the code itself. The bug that led to this code was tricky - maybe add a bit more explanation? The method is general-purpose tho, any optimizer might do it. Are you saying I should just remove the doc? http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java#newcode371 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:371: * Magic magic magic: don't allow code flow from the AsyncFragmentLoader On 2011/06/02 16:36:44, zundel wrote: (again, not magic...) PFM, baby. http://gwt-code-reviews.appspot.com/1442807/diff/6025/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/6025/dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java#newcode66 dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java:66: * Magic magic magic: don't optimizations on the calls from from Will fix the comment. It's magic in the sense that there's a subtle and specific hack in a specific optimizer to violate what that optimizer would ordinary do, for some higher purpose of which the optimizer knows not. From the optimizer's point of view, it's pure magic. To the extent that this jumps out at you as something crazy and possibly wrong-- that's what's it's SUPPOSED to do. :) http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
You introduced a new concept of a volatile polymorphism. Please document it a little better than a comment hidden away that says, Magic... http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
Added comments. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
Hey guys, I ran into some issues because my previous patch was lazy. I was avoiding optimizing ALL calls to runAsync.onSuccess() in TypeTightener and MethodCallTightener. Turns out that some projects *depending* on those calls being optimized. So I went back and added a way to unambiguously mark a particular call don't optimize this!. I went the enum route to avoid adding yet another boolean flag that would increase net memory consumption of JMethodCall. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
http://gwt-code-reviews.appspot.com/1442807/diff/5033/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/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode306 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:306: private final Object[][] allCallbacks; Hehe, because you end up having to do setCheck calls in various places, as opposed to a simple cast check in one or two places. Darn Java semantics. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
Hey guys, I fixed some bugs and optimized a few things. Looking to resubmit this. http://gwt-code-reviews.appspot.com/1442807/diff/5033/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/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode306 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:306: private final Object[][] allCallbacks; This actually generates less code. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
LGTM http://gwt-code-reviews.appspot.com/1442807/diff/5033/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/5033/user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java#newcode306 user/src/com/google/gwt/core/client/impl/AsyncFragmentLoader.java:306: private final Object[][] allCallbacks; On 2011/05/26 21:11:51, scottb wrote: This actually generates less code. I'll bite. Why does it generate less code? http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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();
location vs. getName(). Has the concept changed?
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#newcode55
dev/core/src/com/google/gwt/dev/jdt/FindDeferredBindingSitesVisitor.java:55:
public static final String ASYNC_MAGIC_METHOD = runAsync;
Unused?
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
The upside is that the actual value doesn't matter except that it's
unique across the compilation?
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);
:-)
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. :)
Isn't that pointless? Shouldn't these non-fragments get pruned?
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) {
getQualifiedJsniName() instead?
http://gwt-code-reviews.appspot.com/1442807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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?
Since that string can either be an arbitrary class literal name or the
comiler generated location, probably the name 'location' needs to change
to 'name.
See ReplaceRunAsyncs.AsyncCreateVisitor
I don't know why, but I've seen a lot of code pass a class literal
option. Maybe passing the class literal is a legacy thing and should be
deprecated?
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) {
On 2011/05/23 14:42:41, bobv wrote:
getQualifiedJsniName() instead?
add this as a method on JMethod?
http://gwt-code-reviews.appspot.com/1442807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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
ListJRunAsync 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(ListJRunAsync 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
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
LGTM 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); no, not worth it. http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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#newcode1015
dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java:1015: public void
setRunAsyncs(ListJRunAsync runAsyncs) {
Just curious, was thinking about this as I was looking at the other
current Pruner patch. Is it possible for the set of runAsync fragments
to decrease, say if a GWT.runAsync() call site gets pruned? Do we
account for that possibility? Or do we always rescue all split-points
regardless? (that's what I think I'm seeing). Not sure how often a
developer introduces unreachable splitpoints (or relegates them as dead
when not cleaning up legacy functionality). But it could be something
to consider, here or as a separate patch.
http://gwt-code-reviews.appspot.com/1442807/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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); (After writing this comment I realized it really gets substituted by ReplaceRunAsyncs, I was just looking for refernces to JRunAsync.getRunAsycnCall()) http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
Yeah, sorry. old = what is now committed and new = this patch. On Fri, May 20, 2011 at 9:41 AM, zun...@google.com wrote: I'm missing some contect here. I don't know what you mean by 'new' and old implementations of runAsync. How new is new (this cl? something already committed or a project in the works?) http://gwt-code-reviews.appspot.com/1442807/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
New patch, and added some hopefully helpful commentary. 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); This was an indexed type anyway. 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#newcode92 dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java:92: private JMethod currentMethod; This is needed because the whole stack of methods isn't tracked when dependency recorder isn't running. 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#newcode1691 dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java:1691: gwtOnLoad.setArtificiallyRescued(true); This turns out to be a much better signal to JsUnusedFunctionRemover. 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; Some fragments are now so small, they don't contain any functions. :) http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java File dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java (right): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java#newcode677 dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java:677: refType = nullifyArrayType(arrayType); I ran into an oscillation bug here, because in one test, RunAsyncCallback[][] kept getting transformed to null[] and back due to RunAsyncCall[] being uninstantiable. But, we don't modify the corresponding JNewArray that was flowing into this expression. Seems like we should either type-tighten JNewArray, or else not do this at all. But this fixes the immediate bug... possibly by disabling the optimization in all cases. 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); This was just doing a more work than it needed to. http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/RunAsyncNameTest.java File dev/core/test/com/google/gwt/dev/jjs/impl/RunAsyncNameTest.java (left): http://gwt-code-reviews.appspot.com/1442807/diff/4001/dev/core/test/com/google/gwt/dev/jjs/impl/RunAsyncNameTest.java#oldcode64 dev/core/test/com/google/gwt/dev/jjs/impl/RunAsyncNameTest.java:64: builder.expectError(Cannot proceed due to previous errors, null); Because this is now caught by ReplaceRunAsync instead of FindDeferredBindingSitesVisitor. 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(); 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. 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):
[gwt-contrib] Re: Re-implement runAsync to improve code size. (issue1442807)
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
ListJRunAsync 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,
