[gwt-contrib] Re: initial load sequence for runAsync's
LGTM http://gwt-code-reviews.appspot.com/33848/diff/4002/4005 File dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java (right): http://gwt-code-reviews.appspot.com/33848/diff/4002/4005#newcode68 Line 68: String location = splitPointMap.get(sp); assert location != null http://gwt-code-reviews.appspot.com/33848/diff/4002/4012 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/33848/diff/4002/4012#newcode234 Line 234: private ListInteger splitPointInitialSequence = new ArrayListInteger(); Use Lists.create()? http://gwt-code-reviews.appspot.com/33848 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: initial load sequence for runAsync's
Committed at r5560. http://gwt-code-reviews.appspot.com/33848/diff/4002/4005 File dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java (right): http://gwt-code-reviews.appspot.com/33848/diff/4002/4005#newcode68 Line 68: String location = splitPointMap.get(sp); On 2009/06/15 17:45:30, bobv wrote: assert location != null Done. http://gwt-code-reviews.appspot.com/33848/diff/4002/4012 File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right): http://gwt-code-reviews.appspot.com/33848/diff/4002/4012#newcode234 Line 234: private ListInteger splitPointInitialSequence = new ArrayListInteger(); On 2009/06/15 17:45:30, bobv wrote: Use Lists.create()? Done. http://gwt-code-reviews.appspot.com/33848 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: initial load sequence for runAsync's
Thanks, Bob! I have several things to change; my plans are in the
line-by-line comments.
http://gwt-code-reviews.appspot.com/33848/diff/1/4
File
dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/4#newcode107
Line 107: MapInteger, String names = new HashMapInteger, String();
I'll sort them. It will work to have recordSplitPoints() loop from
1..size instead of looping over the map's key set.
http://gwt-code-reviews.appspot.com/33848/diff/1/5
File dev/core/src/com/google/gwt/core/linker/SoycReportLinker.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/5#newcode43
Line 43: if (soycFiles.getDepFile() != null) {
It's a new requirement. A StandardCompilationAnalysis is now always
added even when -soyc is turned off, because it's both useful and fast
to generate. When -soyc is off, the SCA will include the split point
map but not the other two files.
http://gwt-code-reviews.appspot.com/33848/diff/1/10
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode327
Line 327:
Will do.
http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode363
Line 363: System.out.println(Completed SOYC phase in
Will do. I missed that one.
http://gwt-code-reviews.appspot.com/33848/diff/1/11
File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right):
http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode231
Line 231: private MapInteger, RunAsyncReplacement runAsyncReplacements
= new HashMapInteger, RunAsyncReplacement();
It could be d.u.c.Maps.create(). The setter might not be called, in
which case an empty map leads to simpler code.
http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode1051
Line 1051: splitPointInitialSequence.addAll(list);
It should only be called once. I'll add an assert that the existing
sequence is empty.
http://gwt-code-reviews.appspot.com/33848/diff/1/6
File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right):
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode340
Line 340: * TODO(spoon) accept a labeled runAsync call, once runAsyncs
can be labeled
Some kind of annotation at the runAsync call site. The details are TBD.
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode349
Line 349: + PROP_INITIAL_SEQUENCE + : + refString);
Yes!
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode380
Line 380: Method includes multiple runAsync calls, so it's ambiguous
which one is meant:
On 2009/06/10 23:08:17, bobv wrote:
Split the string literal to improve formatting.
Sure.
This happens after pruning, right?
I'll pull up the code and double check, but it shouldn't be called after
optimization starts. It should run pre-optimization, so that the jsni
references can actually be looked up. This method converts the JSNI
reference to an integer, which will be stable for the whole compile.
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode451
Line 451: MapJMethod, ListInteger revmap = new
java.util.HashMapJMethod, ListInteger();
I'll change it to use GWT's hash map.
http://gwt-code-reviews.appspot.com/33848/diff/1/8
File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/8#newcode36
Line 36: * by calls to a fragment loader.
Will delete.
http://gwt-code-reviews.appspot.com/33848
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: initial load sequence for runAsync's
How about now? In addition to the requested changes, I changed two other things: 1. The SOYC timings use PerfLogger now, rather than inlined timing code 2. There is a check for the same split point being specified multiple times in the initial sequence http://gwt-code-reviews.appspot.com/33848 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: initial load sequence for runAsync's
The one thing that really stands out is that this has an
optimization-dependent failure mode where a runAsync call could get
optimized out of a method.
http://gwt-code-reviews.appspot.com/33848/diff/1/4
File
dev/core/src/com/google/gwt/core/ext/soyc/impl/SplitPointRecorder.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/4#newcode107
Line 107: MapInteger, String names = new HashMapInteger, String();
The use of an ordered map would make the logging statement above easier
to read if you're digging through the trace messages.
http://gwt-code-reviews.appspot.com/33848/diff/1/5
File dev/core/src/com/google/gwt/core/linker/SoycReportLinker.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/5#newcode43
Line 43: if (soycFiles.getDepFile() != null) {
Is this a defensive check or a bugfix?
http://gwt-code-reviews.appspot.com/33848/diff/1/10
File dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode327
Line 327:
Could you extract the SOYC report setup code into its own method? This
one is starting to get a bit long.
http://gwt-code-reviews.appspot.com/33848/diff/1/10#newcode363
Line 363: System.out.println(Completed SOYC phase in
System.out - logger?
http://gwt-code-reviews.appspot.com/33848/diff/1/11
File dev/core/src/com/google/gwt/dev/jjs/ast/JProgram.java (right):
http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode231
Line 231: private MapInteger, RunAsyncReplacement runAsyncReplacements
= new HashMapInteger, RunAsyncReplacement();
Can this be null or dev.util.collect.Maps.create()? The setter below
simply reassigns the field.
http://gwt-code-reviews.appspot.com/33848/diff/1/11#newcode1051
Line 1051: splitPointInitialSequence.addAll(list);
Should this method be called set or append?
http://gwt-code-reviews.appspot.com/33848/diff/1/6
File dev/core/src/com/google/gwt/dev/jjs/impl/CodeSplitter.java (right):
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode340
Line 340: * TODO(spoon) accept a labeled runAsync call, once runAsyncs
can be labeled
The label would be some kind of name-bearing annotation on the callback
type?
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode349
Line 349: + PROP_INITIAL_SEQUENCE + : + refString);
Do you want to throw UnableToCompleteException here?
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode380
Line 380: Method includes multiple runAsync calls, so it's ambiguous
which one is meant:
Split the string literal to improve formatting.
This happens after pruning, right? So it's possible that a developer
could write a method containing multiple runAsync calls that would
sometimes pass/fail depending on optimization. It's probably not
worthwhile to add logic to check for this before pruning, but changing
the message to After optimization, this method includes...
http://gwt-code-reviews.appspot.com/33848/diff/1/6#newcode451
Line 451: MapJMethod, ListInteger revmap = new
java.util.HashMapJMethod, ListInteger();
Why the fully-qualified name here?
http://gwt-code-reviews.appspot.com/33848/diff/1/8
File dev/core/src/com/google/gwt/dev/jjs/impl/ReplaceRunAsyncs.java
(right):
http://gwt-code-reviews.appspot.com/33848/diff/1/8#newcode36
Line 36: * by calls to a fragment loader.
This double-quote seems to have been out of place in the initial code.
http://gwt-code-reviews.appspot.com/33848
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
