[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Colin Alworth has posted comments on this change.
Change subject: Ensure clinits get called for JSO instance methods.
..
Patch Set 2:
(1 comment)
File dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
Line 172: if (program.isStaticImpl(x.getTarget())
!program.isJavaScriptObject(x.getTarget().getEnclosingType())) {
The extra condition (both here and in GenerateJavaScriptAST) deals with the
fact that MakeCallsStatic registers instance methods as static with
JProgram, while JsoDevirtualizer does not. This seems reasonable at first,
though since JSOs are *always* made static, there might be other
implications. I suggested this in my first comment in this change request,
but John seemed to think that this approach made more sense. That said, I
haven't actually tried making that change, and seeing what else is affected
(i.e. all other callsites of JProgram.isStaticImpl or .getStaticImpl).
--
To view, visit https://gwt-review.googlesource.com/3361
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
Gerrit-PatchSet: 2
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: John A. Tamplin j...@jaet.org
Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org
Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Roberto Lublinerman rlu...@google.com
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Matthew Dempsky has posted comments on this change.
Change subject: Ensure clinits get called for JSO instance methods.
..
Patch Set 2:
(3 comments)
File user/test/com/google/gwt/dev/jjs/test/JsoTest.java
Line 49: static final class ClinitStaticFieldFirst extends
JavaScriptObject {
You have three identical class definitions here. I suspect that's because
you have multiple test scenarios, and clinits aren't undoable. It would be
nice to have a comment explaining that, so people don't come later on and
think they can refactor the code and eliminate the extra tests. :)
Line 54: if (FIELD == null) {
What's the point of all these 'if' statements? It looks like you should be
able to eliminate them.
If they're necessary, can you please add a comment explaining why?
Line 84: static final class ClinitStaticMethodFirst extends
JavaScriptObject {
Nit: Blank line before new class definition.
--
To view, visit https://gwt-review.googlesource.com/3361
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
Gerrit-PatchSet: 2
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: John A. Tamplin j...@jaet.org
Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org
Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Roberto Lublinerman rlu...@google.com
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Colin Alworth has posted comments on this change.
Change subject: Ensure clinits get called for JSO instance methods.
..
Patch Set 2:
(2 comments)
File user/test/com/google/gwt/dev/jjs/test/JsoTest.java
Line 49: static final class ClinitStaticFieldFirst extends
JavaScriptObject {
Correct - the commit message documents this, but I'll stick a comment too.
Line 54: if (FIELD == null) {
Again, commit message touched on it. The bug only manifests in compiled
code, and the compiler will inline this if it is too simple. I was going
for the simplest code I could write that would not be optimized out (at
least in draft mode) without using JSNI. Not that there is an issue with
JSNI here, just that I wanted to keep the issue on track for following Java
semantics. Once the method gets inlined, the bug goes away, since the
outside code is accessing the field, and any static field access requires a
clinit (which the compiler doesn't forget before this patch).
--
To view, visit https://gwt-review.googlesource.com/3361
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
Gerrit-PatchSet: 2
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: John A. Tamplin j...@jaet.org
Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org
Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Roberto Lublinerman rlu...@google.com
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Roberto Lublinerman has posted comments on this change.
Change subject: Ensure clinits get called for JSO instance methods.
..
Patch Set 2: Code-Review+1
(1 comment)
File dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
Line 172: if (program.isStaticImpl(x.getTarget())
!program.isJavaScriptObject(x.getTarget().getEnclosingType())) {
Is the extra condition needed? I am not too familiar with how the compiler
handles JSOs but it seems odd to me that statified methods are treated
differently.
--
To view, visit https://gwt-review.googlesource.com/3361
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
Gerrit-PatchSet: 2
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: Colin Alworth niloc...@gmail.com
Gerrit-Reviewer: John A. Tamplin j...@jaet.org
Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org
Gerrit-Reviewer: Matthew Dempsky mdemp...@google.com
Gerrit-Reviewer: Roberto Lublinerman rlu...@google.com
Gerrit-HasComments: Yes
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Hello Leeroy Jenkins, I'd like you to reexamine a change. Please visit https://gwt-review.googlesource.com/3361 to look at the new patch set (#2). Change subject: Ensure clinits get called for JSO instance methods. .. Ensure clinits get called for JSO instance methods. In following Java semantics, any static initialization code in a class (collectively a clinit) should be run before any part of that class is used. To achieve this, GWT ensures that this clinit was invoked before any (actually) static method, and as part of any constructor. This way, instance methods are sure to have already run the clinit, so it doesn't need to be run again. However, JSO constructors don't exist, so instance methods are not guaranteed to have had the clinit already run. This is the bug that this patch is trying to fix. Two locations are changed, both with the effect of ensuring that a clinit is emitted for JSOs. First, the MethodInliner must not skip adding a call to the clinit when the jso body is inlined and the method invocation (and ref to the class) is dropped, and second, GenerateJavaScriptAST must likewise write out explicit clinits for any static method or jso instance method. The jso methods for the unit tests include methods that seem slightly convoluted to avoid being inlined - once inlined, the clinit would re-appear (as it shoud, since the static field is being accessed directly). Each situation being tested requires its own type, since the test app would require being restarted to empty out each static field. Note that prior to this fix, only the instance method case failed and returned null - the other situations are added just to ensure that everything else is behaving as expected going forward. Bug: issue 3192 Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb --- M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java M dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java M user/test/com/google/gwt/dev/jjs/test/JsoTest.java 3 files changed, 70 insertions(+), 4 deletions(-) -- To view, visit https://gwt-review.googlesource.com/3361 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb Gerrit-PatchSet: 2 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: John A. Tamplin j...@jaet.org Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
John A. Tamplin has posted comments on this change. Change subject: Ensure clinits get called for JSO instance methods. .. Patch Set 2: Code-Review+2 -- To view, visit https://gwt-review.googlesource.com/3361 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: comment Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb Gerrit-PatchSet: 2 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: John A. Tamplin j...@jaet.org Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Colin Alworth has uploaded a new change for review.
https://gwt-review.googlesource.com/3361
Change subject: Ensure clinits get called for JSO instance methods.
..
Ensure clinits get called for JSO instance methods.
In following Java semantics, any static initialization code in a class
(collectively a clinit) should be run before any part of that class is
used. To
achieve this, GWT ensures that this clinit was invoked before any (actually)
static method, and as part of any constructor. This way, instance methods
are
sure to have already run the clinit, so it doesn't need to be run again.
However, JSO constructors don't exist, so instance methods are not
guaranteed to
have had the clinit already run. This is the bug that this patch is trying
to
fix.
Two locations are changed, both with the effect of ensuring that a clinit is
emitted for JSOs. First, the MethodInliner must not skip adding a call to
the
clinit when the jso body is inlined and the method invocation (and ref to
the
class) is dropped, and second, GenerateJavaScriptAST must likewise write out
explicit clinits for any static method or jso instance method.
Bug: issue 3192
Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
---
M dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
M dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git
a/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
index 240f6a0..1c7944a 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java
@@ -2189,10 +2189,10 @@
if (!crossClassTargets.contains(x)) {
return null;
}
- if (x.canBePolymorphic() || program.isStaticImpl(x)) {
+ JDeclaredType enclosingType = x.getEnclosingType();
+ if (x.canBePolymorphic() || (program.isStaticImpl(x)
!program.isJavaScriptObject(enclosingType))) {
return null;
}
- JDeclaredType enclosingType = x.getEnclosingType();
if (enclosingType == null || !enclosingType.hasClinit()) {
return null;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
index d2b05ae..6acbb49 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
@@ -169,8 +169,8 @@
// Access from this class to the target class won't trigger a
clinit
return null;
}
- if (program.isStaticImpl(x.getTarget())) {
-// No clinit needed; target is really an instance method.
+ if (program.isStaticImpl(x.getTarget())
!program.isJavaScriptObject(x.getTarget().getEnclosingType())) {
+// No clinit needed; target is really a non-jso instance method.
return null;
}
if (JProgram.isClinit(x.getTarget())) {
--
To view, visit https://gwt-review.googlesource.com/3361
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Colin Alworth niloc...@gmail.com
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups GWT Contributors group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
John A. Tamplin has posted comments on this change. Change subject: Ensure clinits get called for JSO instance methods. .. Patch Set 1: This seems a lot less than what you showed me at the Meet-up -- what did you change? Also needs tests. -- To view, visit https://gwt-review.googlesource.com/3361 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: comment Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb Gerrit-PatchSet: 1 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: John A. Tamplin j...@jaet.org Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
Colin Alworth has posted comments on this change. Change subject: Ensure clinits get called for JSO instance methods. .. Patch Set 1: Afraid not, this is the exact same patch we looked at - only difference was that I pulled the sys.out.println I was using to debug the circumstances for this fix. W.r.t. test cases, would those be best in the form of a test JSO that demonstrates the issue, or something that actually builds a sample ast and verifies that JsInvocations or JMethodCalls are created for the clinit? I have the test sample from the original bug report that could be made into a GwtTestCase, but that seems a little more testing the effect than the bug. That said, I'm not seeing (or perhaps not looking in the right place for) a lot of test cases that make sure the generated JS is sane. One last thought - while moving this from SVN to Git, it occurred to me that this same fix could potentially be achieved in another way: In the same way that MakeCallsStatic ties newly static calls to their original instance version via JProgram.putStaticImpl, it appears that JsoDevirtualizer could do the same. This would make these methods show up as needing a clinit when the `program.isStaticImpl(x.getTarget())` check is made (as in MethodInliner.createClinitCall. From your general gwtc point of view, does that seem to be a reasonable (as well as clearer) fix? -- To view, visit https://gwt-review.googlesource.com/3361 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: comment Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb Gerrit-PatchSet: 1 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: John A. Tamplin j...@jaet.org Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
[gwt-contrib] Change in gwt[master]: Ensure clinits get called for JSO instance methods.
John A. Tamplin has posted comments on this change. Change subject: Ensure clinits get called for JSO instance methods. .. Patch Set 1: I think simply having a JSO with static initializers that don't work without this patch would be sufficient. I think your current fix is clearer than making it appear to be a static impl. -- To view, visit https://gwt-review.googlesource.com/3361 To unsubscribe, visit https://gwt-review.googlesource.com/settings Gerrit-MessageType: comment Gerrit-Change-Id: If09885382bcf2d6c149cd8e48bfdd3ae527d67cb Gerrit-PatchSet: 1 Gerrit-Project: gwt Gerrit-Branch: master Gerrit-Owner: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: Colin Alworth niloc...@gmail.com Gerrit-Reviewer: John A. Tamplin j...@jaet.org Gerrit-Reviewer: Leeroy Jenkins jenk...@gwtproject.org Gerrit-HasComments: No -- http://groups.google.com/group/Google-Web-Toolkit-Contributors --- You received this message because you are subscribed to the Google Groups GWT Contributors group. To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
