JavaConversions and GWT(?) problem
Grzegorz Kossakowski
Grzegorz Kossakowski
--
Grzegorz Kossakowski
Stephen Haberman
> Lscala_collection_MapLike$DefaultKeySet_2_classLit is not defined
That's odd. It's working fine for me. Note that I'd merged in GWT trunk
(as of the latest trunk commit in the scalagwt-gwt fork) to my
scala-test branch, so technically I'm using that code + updated GWT
jars. Maybe see if that fixes it?
- Stephen
Grzegorz Kossakowski
--
Grzegorz Kossakowski
Stephen Haberman
> I forgot to mention it's visible only in Dictionary example,
> Showcase.html#CwDictionaryExample
>
> Did you check that one?
Ah, no, I see it now.
- Stephen
Stephen Haberman
> Lscala_collection_MapLike$DefaultKeySet_2_classLit is not defined
Looks to be an issue with code splitting. The classLit is defined, but
in a .js file that isn't loaded before the dictionary .js file. Not sure
why yet.
- Stephen
Stephen Haberman
> Lscala_collection_MapLike$DefaultKeySet_2_classLit is not defined
As an update, this is indeed a code splitting, but I still can't figure
it out, even after spending an inordinate amount of time on it. So far
all I have to show for my efforts is knowing a lot more about how code
splitting works.
The code splitting implementation itself is somewhat of a nightmare to
debug--it uses a stateful visitor to crawl the control flow graph, and
the stack levels get crazy deep (~20 levels of visitor calls for each
1 level of user call).
Anyway, I'm still thinking about it.
- Stephen
Lex Spoon
http://blog.lexspoon.org/2009/07/dead-for-now-dfn-code-splitting.html
http://blog.lexspoon.org/2009/09/exclusively-live-code.html
http://code.google.com/p/google-web-toolkit/wiki/CodeSplitting#The_results_of_code_splitting
In general, the code splitter is in live use for years now, so it
tends to be reliable. Who knows, though. The problem you describe
brings back memories to me of the kinds of bugs I used to fix at my
previous job.
It would clarify a lot to know which fragment the class literal is
being loaded in, and which one it is referenced from. Is either one of
them the leftovers fragment? Things are simpler if not.
Anyway, once you know that, you can figure out *why* the literal
should have been included in an earlier fragment. Once we know that,
we can debug why the compiler didn't make that decision. For example,
there were problems in the past with figuring out which methods
override which other methods; a problem like that will foul up the
code splitter for sure.
I'm not sure what else to add given the general nature of the problem.
I could probably narrow it down pretty quickly if I knew an isolated
way to replicate the problem....
Lex
Stephen Haberman
> In general, the code splitter is in live use for years now, so it
> tends to be reliable.
Oh, sure, I should have clarified, I doubt the bug is in the code
splitter (...well, maybe), it's just that I was debugging through the
code splitter to try and figure out what was wrong with (I assume) our
ASTs that was causing the failure.
> It would clarify a lot to know which fragment the class literal is
> being loaded in, and which one it is referenced from. Is either one of
> them the leftovers fragment? Things are simpler if not.
Neither are in the leftovers.
The class literal immutable.MapLike$ImmutableDefaultKeySet is in
fragment 34, and it extends collection.MapLike$DefaultKeySet literal,
but that literal is in fragment 20.
So, AFAICT, collection.MapLike$DefaultKeySet needs to at least be in the
leftovers. (...or, alternatively, MapLike$ImmutableDefaultKeySet really
isn't needed by sp34, see below.)
Here's a gist with some debug output:
https://gist.github.com/1117073
* sp1 is interesting because it uses no Scala collections, and just
shows how both sp20 and sp34 walk the CFG when executed together.
Visiting sp20's runAsync (CwAbsolutePanel) finds MapLike$DefaultKeySet
(the base class) and LinkedHashMap$$anon$1 (the subclass within
mutable.HashMap).
Visiting sp34's runAsync (CwDictionaryExample) finds
MapLike$ImmutableDefaultKeySet (the subclass within
immutable.MapLike).
* sp20 is the odd case. LinkedHashMap$$anon$1 isn't found (which is
fine given it was touched via the CwAbsolutePanel runAsync before),
but then sp34's MapLike$ImmutableDefaultKeySet is not found either.
* sp34 shows that sp20 successfully claims MapLike$DefaultKeySet and
LinkedHashMap$$anon$1 just like it did in sp1. And
MapLike$ImmutableDefaultKeySet isn't found. So this is all as
expected.
So, sp34 only conditionally claims MapLike$ImmutableDefaultKeySet, if
it's executed in the same CFG as sp20, otherwise not.
It took me a long time to figure out why in sp20, sp34 wasn't claiming
MapLike$ImmutableDefaultKeySet via its CwDictionaryExample's runAsync.
AFAICT, in sp20, the CFG for sp34's CwDictionaryExample gets this far
in the curMethodStack:
com.google.gwt.sample.showcase.client.content.i18n.CwDictionaryExample$$anon$1::onSuccess
com.google.gwt.sample.showcase.client.content.i18n.CwDictionaryExample::$onInitialize
com.google.gwt.sample.showcase.client.GWTConversions$::$dictionary2Map
scala.collection.immutable.Map$::$empty
scala.collection.immutable.Map$EmptyMap$::$clinit
scala.collection.immutable.Map$EmptyMap$::$plus$plus
scala.collection.immutable.MapLike$class::$plus$plus
scala.collection.immutable.MapLike$$anonfun$$plus$plus$1::apply
scala.collection.immutable.MapLike$$anonfun$$plus$plus$1::$apply
scala.collection.immutable.Map$EmptyMap$::$plus
scala.collection.immutable.Map$EmptyMap$::$$plus
scala.collection.immutable.Map$EmptyMap$::$updated
But then stops because Map$Map1 isn't instantiable. My assumption is
that, say in sp1, sp20's runAsync somehow makes Map$Map1 instantiable,
so sp34's runAsync then continues on and finds ImmutableDefaultKeySet.
But then in sp20, since sp20's runAsync is purposefully not called,
Map$Map1 doesn't get marked instantiable, and sp34 never makes it to
realizing that it does include ImmutableDefaultKeySet (which, if it
was able to, it would rescue it's super class of MapLike$DefaultKeySet,
and so the super class would get shunted to the leftovers).
sp34 may not actually need ImmutableDefaultKeySet, I'm not sure.
When running sp20+sp34 at the same time (say in everything, L({}),
ImmutableDefaultKeySet is live. And in generating sp34 by calculating
L({}) - L({}-{34}) it's not, making GWT infer that L({34}) owns it.
...except that when generating sp20 via L({} - {20}), sp34 doesn't speak
up and claim it, so sp20 thinks it can claim the base class as
exclusive.
> I'm not sure what else to add given the general nature of the problem.
> I could probably narrow it down pretty quickly if I knew an isolated
> way to replicate the problem....
I was unable to replicate the problem with a simple Java example using a
base class and two subclasses. If you are really volunteering (please!
:-), the latest GWT code is in the scala-test branch of:
https://github.com/stephenh/google-web-toolkit
And the sample is Greg's javaConversions branch with the gwt jars
updated to match the latest scala-test branch:
https://github.com/stephenh/scalagwt-sample
- Stephen
Grzegorz Kossakowski
> I'm not sure what else to add given the general nature of the problem.I was unable to replicate the problem with a simple Java example using a
> I could probably narrow it down pretty quickly if I knew an isolated
> way to replicate the problem....
base class and two subclasses. If you are really volunteering (please!
:-), the latest GWT code is in the scala-test branch of:
--
Grzegorz Kossakowski
Stephen Haberman
> I'm wondering if you could reproduce it by creating a very small
> sample program that would use just JavaConversions?
That sounds like a good idea. I'll see what I can do.
- Stephen
Stephen Haberman
> For example, there were problems in the past with figuring out which
> methods override which other methods; a problem like that will foul up
> the code splitter for sure.
Oh. For some reason I just read this, but, yeah, one of the methods here
is overridden. I'm still piecing things together, but I'm ~75% sure I'll
have a reproducible case...well, eventually.
Also, just saw this comment in RescueVisitor:
* TODO(later): make RescueVisitor use less stack?
Yes!
Not of a fan of the "later" attribution though. :-)
- Stephen
Stephen Haberman
> For example, there were problems in the past with figuring out which
> methods override which other methods; a problem like that will foul up
> the code splitter for sure.
Well, I don't think it's a problem with mis-identifying overrides, but
I've reproduced what is happening:
https://github.com/stephenh/scalagwt-sample/commit/6cc1b70ae18c31063f71ce27fc110646c65c73c0
FragmentA:
* Makes Bar.go() alive (which defers the BarSub.go override).
* Calls new Baz()
FragmentB:
* Calls new BarSub() (which makes BarSub instantiable, but go isn't
alive)
When executing together (e.g. for everything), BarSub.go is both
live+instantiable, so BarSub.go is rescued and BazSub is instantiated.
So, when FragmentA is calc'd, BarSub isn't instantiable, so FragmentB
doesn't claim SubBaz. GWT thinks FragmentA can own Baz. GWT also thinks
FragmentA can own BazSub.
Then FragmentB is calc'd, .go isn't alive, so FragmentA doesn't claim
SubBaz either. However, because SubBaz is in everything (and FragmentB
came last), GWT thinks FragmentB owns it.
...
Well, dammit, I fixed it. What's ironic is that I distinctly remember
staring at the fixUpLoadOrderDependenciesForClassLiterals method days
ago and thinking "oh, yeah, that should solve my problem". After a few
minutes, I realized it only handled strings, said "huh, okay", and then
moved on to spend hours divining why exactly these two class literals
were ending up in separate fragments.
Of course, in hindsight, it doesn't matter how contrived the scenario
that led to them being in different fragments, it just needs to be fixed
up. Which is actually not that hard:
https://github.com/stephenh/scalagwt-gwt/commit/b6d5bf7b771ed1beb6445c864052fa01f672cd97
So, I dunno, Lex, what do you think? I still find it odd that BazSub
was inferred as exclusive to both FragmentA and FragmentB, and ended up
in FragmentB (because it was last) even though FragmentB didn't really
need it (AFAICT?). I suppose this is just an artifact of the L({}
- {x}) approach?
- Stephen
Stephen Haberman
> I tried to add support for JavaConverions and from jribble
> backend/library side I think it should work.
After applying the code splitting fix, the next problem is that the
same CwDictionary page now fails with a "to be defined" RuntimeException
in:
scala.compat.Platform$.arraycopy
- Stephen
Grzegorz Kossakowski
--
Grzegorz Kossakowski
Stephen Haberman
> Oh, I replaced call to System.arraycopy because I thought GWT doesn't
> support it. Apparently, I was wrong and GWT does, indeed support it.
Cool, thanks.
Now it's failing on:
https://github.com/paulp/scala-full/blob/master/src/library/scala/Array.scala#L94
Because Class.isAssignableFrom is not implemented in GWT's
java.lang.Class implementation.
- Stephen
Grzegorz Kossakowski
--
Grzegorz Kossakowski
Lex Spoon
I share your confusion about why ControlFlowAnalyzer didn't already
take care of this. If you aren't totally sick of this problem yet, it
might be worth tracing through and seeing why a class literal can
become live but not the superclass literal. It should be possible to
make that happen. Whenever a class literal is traced, also trace the
literal for the superclass.
Either way, it would be really good to submit this patch to GWT ASAP.
It's a standalone bug that should occur for Java code as well as for
Scala. If you decide to fix it in ControlFlowAnalyzer, it would be
good to take the time to add a test case to ControlFlowAnalyzerTest.
-Lex
Stephen Haberman
> it might be worth tracing through and seeing why a class literal can
> become live but not the superclass literal.
AFAICT, the superclass literal was always explicitly live when the
subclass literal was explicitly live, this just fell out because of the
implicit nature of the code splitter.
The subclass literal was only hit in L({}), so for each of the split
points, none of them explicitly hit it (if they did, it would have
correctly pulled in/pushed-to-0 the super classlit).
It was only the L({} - {1}) calc that made GWT put the subclass lit into
sp2. But since this was inferred, and not explicitly walked as live, no
checks were in place for the super classlit.
Makes me curious; the class lit field is just:
foo_classLit = createForClass("package", "Name", Bar.class)
So, it's just a field with an initializer. What keeps other fields
from suffering the same fate? Ending up in sp X, but with their
initializer relying on strings/literals/methods in other split
points?
(Specifically in the degenerate case where the field is only
walked in L({}).)
- Stephen
Stephen Haberman
> So, it's just a field with an initializer. What keeps other fields
> from suffering the same fate? Ending up in sp X, but with their
> initializer relying on strings/literals/methods in other split
> points?
Hm. It looks like most fields are fine, because their initializers
stay in their class's clinit.
The class literals are problematic because they are initialized on
split point load, so they are all instantiated immediately even if
some wouldn't be applicable until everything had loaded (the only
scenario where they were reachable, e.g. computeEverythingCfa).
- Stephen
Lex Spoon
just like your patch does.
What's unusual about this case is that there is code that is live only
when both split points 20 and 34 are activated. Any such code is
considered exclusive to both 20 and 34, and it's a toss up which one
any such code will end up in.
Regarding the test case... well it is work that needs to happen before
merge. We should give the GWT team a Jribble patch, not a mega-patch
that both adds Jribble and also fixes some of their bugs. Reviewers
really dislike megapatches.
Lex
Stephen Haberman
> Based on what you describe, this is a call for a load-order fixup,
> just like your patch does.
Cool.
> Regarding the test case... well it is work that needs to happen before
> merge. We should give the GWT team a Jribble patch, not a mega-patch
> that both adds Jribble and also fixes some of their bugs. Reviewers
> really dislike megapatches.
Of course. I've submitted a patch for just this issue:
http://gwt-code-reviews.appspot.com/1513803/
Although it lacks tests. I'd have to put together something like
ControlFlowAnalyzerTest:
sourceOracle.addOrReplace(new MockJavaResource("test.VirtualUpRef")
With the Bar/SubBar/etc. classes from my reproduction, and then some
sort of analyzeSnippet--although reusing assertOnlyLiveFieldsAndMethods
looks doable. Well, cool, I'll see if I can get around to that.
Speaking of avoiding meta-patches, it would be possible to submit the
source name/internal name changes directly to GWT without the rest of
the jribble code. But I'll wait until we get more feedback on that
approach.
- Stephen
Stephen Haberman
> The subclass literal was only hit in L({})
Gah, I was mismusing the L notation. Basically everywhere I've
said "L({})" I meant "L(everything)".
- Stephen
Lex Spoon
<ste...@exigencecorp.com> wrote:
>
>> Based on what you describe, this is a call for a load-order fixup,
>> just like your patch does.
>
> Cool.
>
>> Regarding the test case... well it is work that needs to happen before
>> merge. We should give the GWT team a Jribble patch, not a mega-patch
>> that both adds Jribble and also fixes some of their bugs. Reviewers
>> really dislike megapatches.
>
> Of course. I've submitted a patch for just this issue:
>
> http://gwt-code-reviews.appspot.com/1513803/
Thanks Stephen. This is very helpful.
> Although it lacks tests. I'd have to put together something like
> ControlFlowAnalyzerTest:
>
> sourceOracle.addOrReplace(new MockJavaResource("test.VirtualUpRef")
>
> With the Bar/SubBar/etc. classes from my reproduction, and then some
> sort of analyzeSnippet--although reusing assertOnlyLiveFieldsAndMethods
> looks doable. Well, cool, I'll see if I can get around to that.
Since it's a load order fixup, I don't think a CFA Test will work
after all. At least a small repro case would be helpful to the
reviewer, however.
-Lex