[gwt-contrib] Re: One last memory review
Thanks, Ray (and Bruce). I showed Anna this thread and she totally ROFLed. Then she told me if I don't stop working you guys really will need an insurance policy on me. ;) On Thu, Apr 9, 2009 at 5:26 PM, Ray Cromwell wrote: > > I hope Google takes out an extra insurance policy so that should an > accident occur, they can cryogenically freeze your brain and thaw you out in > the future in case there are any outstanding compiler issues no one has been > able to fix. :) > -Ray > > > On Thu, Apr 9, 2009 at 10:24 AM, Scott Blum wrote: > >> Don't worry, I'm getting plenty of naps in during the hour-long hyperbaric >> sessions I do with Aaron. Which is, good, since we have to get up at 7 to >> make the morning ones. :) But we're having some fun too. Aaron and I just >> got back from hanging out at the pool and soaking up some sun. Aaron >> preferred to lay on the lounge chairs with a shirt over his head, while I >> did laps around the pool on my Ripstick. >> >> >> On Thu, Apr 9, 2009 at 12:37 PM, Bruce Johnson wrote: >> >>> w00t! Vacation commits FTW! (Just kidding; @Scott: please rest) >>> >>> >>> On Thu, Apr 9, 2009 at 11:52 AM, Scott Blum wrote: >>> FYI: I just committed the last of my outstanding memory work to trunk. Lex kindly agreed to watch the build and do a roll-back for me if something breaks. On Tue, Apr 7, 2009 at 10:33 PM, Scott Blum wrote: > On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: > >> > 5178: Also tightened up the recursive method slightly, and managing >> the >> > "computed" set better. This works because once a class transitions >> from >> > hasClinit -> !hasClinit, there's no possible way it can ever go >> back. >> >> Small problem: I believe line 644-646 in the patched version is >> intended to test "target", not "type". If that sounds right, then the >> rest LGTM. Otherwise, let's discuss how this is supposed to work. >> > > Nice catch! I botched a manual inlining of lines 636-641 in the > original. > > >> By the way, this algorithm could be sped up if, it mattered for >> performance. Instead of repeatedly recursing for each type, start by >> marking classes where hasLiveCode() as having clinits. Then, >> propagate clinit-ness backwards along the getClinitTargets() graph. >> Any class not reached does not needs its clinit. The advantage >> probably doesn't matter in this case in practice, but I mention it >> because this funny algorithm pattern keeps coming up. >> > > That is a better approach, I'll have to remember that next time I run > into this pattern. > > Thanks, > Scott > > >>> >>> >>> >> >> >> > > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
I hope Google takes out an extra insurance policy so that should an accident occur, they can cryogenically freeze your brain and thaw you out in the future in case there are any outstanding compiler issues no one has been able to fix. :) -Ray On Thu, Apr 9, 2009 at 10:24 AM, Scott Blum wrote: > Don't worry, I'm getting plenty of naps in during the hour-long hyperbaric > sessions I do with Aaron. Which is, good, since we have to get up at 7 to > make the morning ones. :) But we're having some fun too. Aaron and I just > got back from hanging out at the pool and soaking up some sun. Aaron > preferred to lay on the lounge chairs with a shirt over his head, while I > did laps around the pool on my Ripstick. > > > On Thu, Apr 9, 2009 at 12:37 PM, Bruce Johnson wrote: > >> w00t! Vacation commits FTW! (Just kidding; @Scott: please rest) >> >> >> On Thu, Apr 9, 2009 at 11:52 AM, Scott Blum wrote: >> >>> FYI: I just committed the last of my outstanding memory work to trunk. >>> Lex kindly agreed to watch the build and do a roll-back for me if something >>> breaks. >>> >>> >>> On Tue, Apr 7, 2009 at 10:33 PM, Scott Blum wrote: >>> On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: > > 5178: Also tightened up the recursive method slightly, and managing > the > > "computed" set better. This works because once a class transitions > from > > hasClinit -> !hasClinit, there's no possible way it can ever go back. > > Small problem: I believe line 644-646 in the patched version is > intended to test "target", not "type". If that sounds right, then the > rest LGTM. Otherwise, let's discuss how this is supposed to work. > Nice catch! I botched a manual inlining of lines 636-641 in the original. > By the way, this algorithm could be sped up if, it mattered for > performance. Instead of repeatedly recursing for each type, start by > marking classes where hasLiveCode() as having clinits. Then, > propagate clinit-ness backwards along the getClinitTargets() graph. > Any class not reached does not needs its clinit. The advantage > probably doesn't matter in this case in practice, but I mention it > because this funny algorithm pattern keeps coming up. > That is a better approach, I'll have to remember that next time I run into this pattern. Thanks, Scott >>> >>> >>> >> >> >> > > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
Don't worry, I'm getting plenty of naps in during the hour-long hyperbaric sessions I do with Aaron. Which is, good, since we have to get up at 7 to make the morning ones. :) But we're having some fun too. Aaron and I just got back from hanging out at the pool and soaking up some sun. Aaron preferred to lay on the lounge chairs with a shirt over his head, while I did laps around the pool on my Ripstick. On Thu, Apr 9, 2009 at 12:37 PM, Bruce Johnson wrote: > w00t! Vacation commits FTW! (Just kidding; @Scott: please rest) > > > On Thu, Apr 9, 2009 at 11:52 AM, Scott Blum wrote: > >> FYI: I just committed the last of my outstanding memory work to trunk. >> Lex kindly agreed to watch the build and do a roll-back for me if something >> breaks. >> >> >> On Tue, Apr 7, 2009 at 10:33 PM, Scott Blum wrote: >> >>> On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: >>> > 5178: Also tightened up the recursive method slightly, and managing the > "computed" set better. This works because once a class transitions from > hasClinit -> !hasClinit, there's no possible way it can ever go back. Small problem: I believe line 644-646 in the patched version is intended to test "target", not "type". If that sounds right, then the rest LGTM. Otherwise, let's discuss how this is supposed to work. >>> >>> Nice catch! I botched a manual inlining of lines 636-641 in the >>> original. >>> >>> By the way, this algorithm could be sped up if, it mattered for performance. Instead of repeatedly recursing for each type, start by marking classes where hasLiveCode() as having clinits. Then, propagate clinit-ness backwards along the getClinitTargets() graph. Any class not reached does not needs its clinit. The advantage probably doesn't matter in this case in practice, but I mention it because this funny algorithm pattern keeps coming up. >>> >>> That is a better approach, I'll have to remember that next time I run >>> into this pattern. >>> >>> Thanks, >>> Scott >>> >>> >> >> >> > > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
w00t! Vacation commits FTW! (Just kidding; @Scott: please rest) On Thu, Apr 9, 2009 at 11:52 AM, Scott Blum wrote: > FYI: I just committed the last of my outstanding memory work to trunk. Lex > kindly agreed to watch the build and do a roll-back for me if something > breaks. > > > On Tue, Apr 7, 2009 at 10:33 PM, Scott Blum wrote: > >> On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: >> >>> > 5178: Also tightened up the recursive method slightly, and managing the >>> > "computed" set better. This works because once a class transitions >>> from >>> > hasClinit -> !hasClinit, there's no possible way it can ever go back. >>> >>> Small problem: I believe line 644-646 in the patched version is >>> intended to test "target", not "type". If that sounds right, then the >>> rest LGTM. Otherwise, let's discuss how this is supposed to work. >>> >> >> Nice catch! I botched a manual inlining of lines 636-641 in the original. >> >> >>> By the way, this algorithm could be sped up if, it mattered for >>> performance. Instead of repeatedly recursing for each type, start by >>> marking classes where hasLiveCode() as having clinits. Then, >>> propagate clinit-ness backwards along the getClinitTargets() graph. >>> Any class not reached does not needs its clinit. The advantage >>> probably doesn't matter in this case in practice, but I mention it >>> because this funny algorithm pattern keeps coming up. >>> >> >> That is a better approach, I'll have to remember that next time I run into >> this pattern. >> >> Thanks, >> Scott >> >> > > > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
FYI: I just committed the last of my outstanding memory work to trunk. Lex kindly agreed to watch the build and do a roll-back for me if something breaks. On Tue, Apr 7, 2009 at 10:33 PM, Scott Blum wrote: > On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: > >> > 5178: Also tightened up the recursive method slightly, and managing the >> > "computed" set better. This works because once a class transitions from >> > hasClinit -> !hasClinit, there's no possible way it can ever go back. >> >> Small problem: I believe line 644-646 in the patched version is >> intended to test "target", not "type". If that sounds right, then the >> rest LGTM. Otherwise, let's discuss how this is supposed to work. >> > > Nice catch! I botched a manual inlining of lines 636-641 in the original. > > >> By the way, this algorithm could be sped up if, it mattered for >> performance. Instead of repeatedly recursing for each type, start by >> marking classes where hasLiveCode() as having clinits. Then, >> propagate clinit-ness backwards along the getClinitTargets() graph. >> Any class not reached does not needs its clinit. The advantage >> probably doesn't matter in this case in practice, but I mention it >> because this funny algorithm pattern keeps coming up. >> > > That is a better approach, I'll have to remember that next time I run into > this pattern. > > Thanks, > Scott > > --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
On Tue, Apr 7, 2009 at 4:12 PM, Lex Spoon wrote: > > 5178: Also tightened up the recursive method slightly, and managing the > > "computed" set better. This works because once a class transitions from > > hasClinit -> !hasClinit, there's no possible way it can ever go back. > > Small problem: I believe line 644-646 in the patched version is > intended to test "target", not "type". If that sounds right, then the > rest LGTM. Otherwise, let's discuss how this is supposed to work. > Nice catch! I botched a manual inlining of lines 636-641 in the original. > By the way, this algorithm could be sped up if, it mattered for > performance. Instead of repeatedly recursing for each type, start by > marking classes where hasLiveCode() as having clinits. Then, > propagate clinit-ness backwards along the getClinitTargets() graph. > Any class not reached does not needs its clinit. The advantage > probably doesn't matter in this case in practice, but I mention it > because this funny algorithm pattern keeps coming up. > That is a better approach, I'll have to remember that next time I run into this pattern. Thanks, Scott --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: One last memory review
> 5176: All this does is tweak the debug output of the Java AST. JSNI methods > have proper indentation in an AST dump now (I was using AST dumps to verify > incremental correctness.) LGTM. > 5177: Fairly self-explanatory; added the state to JReferenceType, but kept > it in JTypeOracle at first to assert there was no difference. Aside: I'm > kind of displeased with the type hierarchy of JReferenceType. It really > feels like JClassType and JInterfaceType should share a common ancestor that > JArrayType and JNullType do not. If this were the structure, I'd have put > abstract methods into JReferenceType and put the statefulness in that common > ancestor. LGTM. I see what you mean about JArrayType being an immediate sibling of JClassType; it doesn't really make sense to ask an array type if it has a clinit. > 5178: Also tightened up the recursive method slightly, and managing the > "computed" set better. This works because once a class transitions from > hasClinit -> !hasClinit, there's no possible way it can ever go back. Small problem: I believe line 644-646 in the patched version is intended to test "target", not "type". If that sounds right, then the rest LGTM. Otherwise, let's discuss how this is supposed to work. By the way, this algorithm could be sped up if, it mattered for performance. Instead of repeatedly recursing for each type, start by marking classes where hasLiveCode() as having clinits. Then, propagate clinit-ness backwards along the getClinitTargets() graph. Any class not reached does not needs its clinit. The advantage probably doesn't matter in this case in practice, but I mention it because this funny algorithm pattern keeps coming up. > 5179: I should have actually squashed this tiny fix into a previous commit; > apparently sometimes people were calling hasClinit with nulls. LGTM. It looks like things would be simpler if we gave those last straggling fields and methods an actual enclosing class, but now's not the time. > 5180: Note the assertion in JFieldRef... I added this because I ran into a > null JFieldRef.enclosingType() late in the compile. This assertion > uncovered a major problem... JsniFieldRefs always had a null enclosing type. > I didn't check, but it's possible this could have been resulting in a > failure to clinit a class when one of its fields is first accessed from > JSNI. The fix is in GenerateJavaAST in this patch. LGTM. > 5181: Should be self-explanatory. LGTM. -Lex --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
