[gwt-contrib] Re: RR: slimmed down SOYC + dependencies written to file
Thanks. This LGTM. Minor things that don't need a re(re)review if you do them: ControlFlowAnalyzer.recordDependencies is no longer needed; it's equivalent to "dr != null". The DependencyRecorder interface could be made a static member of ControlFlowAnalyzer. Lex --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: RR: slimmed down SOYC + dependencies written to file
Apologies -- here is the real patch.
kathrin
On Tue, Jan 13, 2009 at 9:43 PM, Katharina Probst wrote:
> Hi Lex,
>
> would you take another pass at the updated patch? Here are the main changes:
>
> 1. The patch has a cleaner implementation of dependency recording by
> adding a DependencyRecorder interface and its implementation. This is
> now called from both JavaToJavaScriptCompiler and from
> ControlFlowAnalysis, so all the dependency recording is gathered in
> one place.
> 2. I added recording of field information back in. This does not add
> too much to the memory requirements. However, as it is not currently
> used in the dashboard, I still consider it as slated for possible
> elimination.
> 3. I added the work directory to the options that are passed through
> to JavaToJavaScriptCompiler.
>
> I have also addressed all your smaller comments with the exception of
> the follow-up work, which I will work on next.
>
> Thanks,
> kathrin
>
> On Fri, Jan 9, 2009 at 2:15 PM, Lex Spoon wrote:
>> This is a really great step forward! I'll list my favorite next steps
>> at the end of the email, but let me say that getting this dependency
>> information is really great for helping people (a) shrink their code,
>> and (b) if they are using runAsync, shrink their initial download.
>>
>> The overall strategy looks great. It's actually simpler than what I
>> was picturing.
>>
>> I do have one question about dropping field information: does this
>> mean we lose the ability to track which fragment a string literal ends
>> up in? If so, it might be premature to drop the fields unless it's
>> really desperately needed. We would end up adding them right back
>> again in a future iteration.
>>
>>
>> possible problems:
>>
>> CompilationAnalysis.compareToComparableArtifact does not match the
>> Comparable contract in the case that one side or the other is null.
>> It needs to be the case that if a.compareTo(b) is -1, then
>> b.compareTo(a) is 1. This could be achieved by replacing the one
>> check with three: both null (0), one side is null (-1), the other is
>> null (1).
>>
>>
>> style things:
>>
>> JavaToJavaScriptCompiler.workDirectory is a mutable static field,
>> which could lead to problems. Sometimes there are multiple compilers
>> running in the same class loader. Anyway, the work directory should
>> really be available in the options. If it's not available there
>> already, let's figure out how to get it in there.
>>
>> In JavaToJavaScriptCompiler, the block of code that emits the
>> dependency information should be factored out into its own method.
>> Better, put it in a utility class for printing out dependencies,
>> analogous to how the other compiler phases work.
>>
>> ControlFlowAnalyzer should not include HTML generation. It looks much
>> cleaner for it to define an interface DependencyRecorder and to let
>> the caller specify a recorder, which for now will directly emit HTML.
>> The actual HTML printing would then be in the utility class for
>> recording dependency information. The DependencyRecorder interface
>> would be defined something like:
>>
>> interface DependencyRecorder {
>>void methodIsLiveBecause(JMethod liveMethod, Iterable
>> dependencyChain);
>> }
>>
>> In SoycReportLinker.java, "reports.size() > 1" is easier to read than
>> "!(reports.size() == 1))".
>>
>> In CompilationAnalysis.java, that's an improvement to use @return, but
>> the word "Returns" should also be dropped. Javadoc should fill it in.
>>
>> The following files need formatting to remove extra spaces:
>> GenerateJavaScriptAST.java, BuildTypeMap.java, CompilePerms.java.
>>
>>
>>
>> Finally, there are some easy followups that it would be great to do as
>> later iterations, but not much later:
>>
>> 1. Record the top of the method stack in a few key places: when a
>> class becomes instantiable, and when a string is rescued. People will
>> be asking, "Why is B.foo() considered callable, even though B should
>> not be instantiable?" Likewise, they'll ask "why is this humongous
>> message string included in the initial download, even though it
>> shouldn't be used until after a split point is reached?
>>
>> 2. The approach of writing directly to disk might cause trouble for
>> sharded compiles. Please test on a sharded compile. If it works,
>> that's great. If it doesn't work, I don't actually think it needs to
>> work, but at least it should gracefully error out.
>>
>> -Lex
>>
>
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
Index: dev/core/src/com/google/gwt/core/ext/linker/impl/StandardCompilationAnalysis.java
===
--- dev/core/src/com/google/gwt/core/ext/linker/impl/StandardCompilationAnalysis.java (revision 4424)
+++ dev/core/src/com/google/gwt/core/ext/linker/impl/StandardCompilationAnalysis.java (working copy)
@@ -25,
[gwt-contrib] Re: RR: slimmed down SOYC + dependencies written to file
Hi Lex,
would you take another pass at the updated patch? Here are the main changes:
1. The patch has a cleaner implementation of dependency recording by
adding a DependencyRecorder interface and its implementation. This is
now called from both JavaToJavaScriptCompiler and from
ControlFlowAnalysis, so all the dependency recording is gathered in
one place.
2. I added recording of field information back in. This does not add
too much to the memory requirements. However, as it is not currently
used in the dashboard, I still consider it as slated for possible
elimination.
3. I added the work directory to the options that are passed through
to JavaToJavaScriptCompiler.
I have also addressed all your smaller comments with the exception of
the follow-up work, which I will work on next.
Thanks,
kathrin
On Fri, Jan 9, 2009 at 2:15 PM, Lex Spoon wrote:
> This is a really great step forward! I'll list my favorite next steps
> at the end of the email, but let me say that getting this dependency
> information is really great for helping people (a) shrink their code,
> and (b) if they are using runAsync, shrink their initial download.
>
> The overall strategy looks great. It's actually simpler than what I
> was picturing.
>
> I do have one question about dropping field information: does this
> mean we lose the ability to track which fragment a string literal ends
> up in? If so, it might be premature to drop the fields unless it's
> really desperately needed. We would end up adding them right back
> again in a future iteration.
>
>
> possible problems:
>
> CompilationAnalysis.compareToComparableArtifact does not match the
> Comparable contract in the case that one side or the other is null.
> It needs to be the case that if a.compareTo(b) is -1, then
> b.compareTo(a) is 1. This could be achieved by replacing the one
> check with three: both null (0), one side is null (-1), the other is
> null (1).
>
>
> style things:
>
> JavaToJavaScriptCompiler.workDirectory is a mutable static field,
> which could lead to problems. Sometimes there are multiple compilers
> running in the same class loader. Anyway, the work directory should
> really be available in the options. If it's not available there
> already, let's figure out how to get it in there.
>
> In JavaToJavaScriptCompiler, the block of code that emits the
> dependency information should be factored out into its own method.
> Better, put it in a utility class for printing out dependencies,
> analogous to how the other compiler phases work.
>
> ControlFlowAnalyzer should not include HTML generation. It looks much
> cleaner for it to define an interface DependencyRecorder and to let
> the caller specify a recorder, which for now will directly emit HTML.
> The actual HTML printing would then be in the utility class for
> recording dependency information. The DependencyRecorder interface
> would be defined something like:
>
> interface DependencyRecorder {
>void methodIsLiveBecause(JMethod liveMethod, Iterable
> dependencyChain);
> }
>
> In SoycReportLinker.java, "reports.size() > 1" is easier to read than
> "!(reports.size() == 1))".
>
> In CompilationAnalysis.java, that's an improvement to use @return, but
> the word "Returns" should also be dropped. Javadoc should fill it in.
>
> The following files need formatting to remove extra spaces:
> GenerateJavaScriptAST.java, BuildTypeMap.java, CompilePerms.java.
>
>
>
> Finally, there are some easy followups that it would be great to do as
> later iterations, but not much later:
>
> 1. Record the top of the method stack in a few key places: when a
> class becomes instantiable, and when a string is rescued. People will
> be asking, "Why is B.foo() considered callable, even though B should
> not be instantiable?" Likewise, they'll ask "why is this humongous
> message string included in the initial download, even though it
> shouldn't be used until after a split point is reached?
>
> 2. The approach of writing directly to disk might cause trouble for
> sharded compiles. Please test on a sharded compile. If it works,
> that's great. If it doesn't work, I don't actually think it needs to
> work, but at least it should gracefully error out.
>
> -Lex
>
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
Write file: ~/work/gwt-trunk-soyc/soyc-skinny-withdependencies1_r4424.patch
[gwt-contrib] Re: RR: slimmed down SOYC + dependencies written to file
This is a really great step forward! I'll list my favorite next steps
at the end of the email, but let me say that getting this dependency
information is really great for helping people (a) shrink their code,
and (b) if they are using runAsync, shrink their initial download.
The overall strategy looks great. It's actually simpler than what I
was picturing.
I do have one question about dropping field information: does this
mean we lose the ability to track which fragment a string literal ends
up in? If so, it might be premature to drop the fields unless it's
really desperately needed. We would end up adding them right back
again in a future iteration.
possible problems:
CompilationAnalysis.compareToComparableArtifact does not match the
Comparable contract in the case that one side or the other is null.
It needs to be the case that if a.compareTo(b) is -1, then
b.compareTo(a) is 1. This could be achieved by replacing the one
check with three: both null (0), one side is null (-1), the other is
null (1).
style things:
JavaToJavaScriptCompiler.workDirectory is a mutable static field,
which could lead to problems. Sometimes there are multiple compilers
running in the same class loader. Anyway, the work directory should
really be available in the options. If it's not available there
already, let's figure out how to get it in there.
In JavaToJavaScriptCompiler, the block of code that emits the
dependency information should be factored out into its own method.
Better, put it in a utility class for printing out dependencies,
analogous to how the other compiler phases work.
ControlFlowAnalyzer should not include HTML generation. It looks much
cleaner for it to define an interface DependencyRecorder and to let
the caller specify a recorder, which for now will directly emit HTML.
The actual HTML printing would then be in the utility class for
recording dependency information. The DependencyRecorder interface
would be defined something like:
interface DependencyRecorder {
void methodIsLiveBecause(JMethod liveMethod, Iterable
dependencyChain);
}
In SoycReportLinker.java, "reports.size() > 1" is easier to read than
"!(reports.size() == 1))".
In CompilationAnalysis.java, that's an improvement to use @return, but
the word "Returns" should also be dropped. Javadoc should fill it in.
The following files need formatting to remove extra spaces:
GenerateJavaScriptAST.java, BuildTypeMap.java, CompilePerms.java.
Finally, there are some easy followups that it would be great to do as
later iterations, but not much later:
1. Record the top of the method stack in a few key places: when a
class becomes instantiable, and when a string is rescued. People will
be asking, "Why is B.foo() considered callable, even though B should
not be instantiable?" Likewise, they'll ask "why is this humongous
message string included in the initial download, even though it
shouldn't be used until after a split point is reached?
2. The approach of writing directly to disk might cause trouble for
sharded compiles. Please test on a sharded compile. If it works,
that's great. If it doesn't work, I don't actually think it needs to
work, but at least it should gracefully error out.
-Lex
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
