[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: SA: Various fixes to sa.js to make it work in JDK8
From: Krystal Mok <rednaxelafx () gmail ! com>
Date: 2013-07-31 14:11:57
Message-ID: CA+cQ+tQk2SF2txPoP_0ZyWUXtcxaqh58Ux-L8TkiRbV64R1NpQ () mail ! gmail ! com
[Download RAW message or body]
Hi all,
I've just learned that there's a bug number that's exactly what I was
trying to fix with this patch:
Bug 8011888: http://bugs.sun.com/view_bug.do?bug_id=8011888
If there's already a fix to this bug then I'm fine with it. Just like to
see the fix in mainline tip so that CLHSDB would work out of the box again.
Thanks,
Kris
On Mon, Jul 29, 2013 at 3:53 PM, Krystal Mok <rednaxelafx@gmail.com> wrote:
> Hi all,
>
> Could I have a couple of reviews for this patch, please?
>
> https://gist.github.com/rednaxelafx/6102608
>
> Switching the JavaScript engine from Rhino to Nashorn and the No PermGen
> project caused a few issues that stopped sa.js from working. This is a
> patch that tries to fix the issues that I've run into.
>
> I'll walkthrough the patch below: (line numbers refer to sajs.patch, not
> sa.js)
>
> Line 8-12:
>
> Since Nashorn implements ECMAScript 5.1, using JavaScript keywords as the
> property name in the "dot" syntax is not a problem anymore. So I'm
> un-commenting these lines.
>
> Line 20-42, 51-76:
>
> When implementing a JavaAdapter for SA ScriptObject, the __get__ function
> calls the __has__ function:
> this.__has__(name)
> which is working at the wrong level: __has__ is a special hook function,
> and cannot be called via "this" this way. It'll trigger the __call__ hook
> to find the "__has__" member, but the JavaAdapter here does not override
> __call__, and then the lookup will fail.
>
> Rather than going through the trouble of implementing the __call__ hook
> just for this purpose, I move the __has__ function up, and made __get__
> call __has__ directly instead. Now it won't trigger the __call__ hook for
> the lookup, the things will work fine again.
>
> Line 45-48:
>
> Removed trailing whitespace.
>
> Line 84-85:
>
> This code used to work in Rhino, but apparently Nashorn doesn't
> automatically convert a JavaScript Array into a Java array when doing
> JS-to-Java interop, so this conversion has to be done manually.
>
> Line 93-104:
>
> I'm a little confused in this part. Nashorn exposes a "print" function
> that's defined in jdk/nashorn/api/scripting/resources/engine.js, and that
> it doesn't expose a corresponding "println" function. I'm not sure if this
> code really worked when using Rhino...anyway, the "println" function is
> missing, so "writeln = println" doesn't do anything useful. I'm adding a
> shim here just in case either of "print" or "println" functions are
> missing. This fixes an error when calling "println" in line 87 of this
> patch.
>
> Line 113:
>
> This is the same change as purposed by Yunda back in April. [1] A missing
> fix from the NPG changes.
>
> Line 121-122, 129-130:
>
> When specifying a method overload in Nashorn, the argument syntax it takes
> for inner classes uses "." as the separator between the enclosing class
> name and the inner class name, instead of "$" as in the "binary name".
>
> Line 138-144, 152-160:
>
> Nashorn has stricter default behaviors than Rhino when overriding Java
> methods. Where as Rhino defaults to a "do nothing" implementation, Nashorn
> defaults to throwing UnsupportedOperationException for unimplemented
> methods.
>
> Line 168-172:
>
> Before the fix, this code only replace the first occurrence of the
> specified special characters, which happens to be not enough for newer
> HotSpot types in C++ templates.
>
> That's all for this patch.
>
> BTW, there is another patches pending review, too: JDK-8011979 [2]
>
> Thanks,
> Kris
>
> [1]:
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html
> [2]:
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html
>
[Attachment #3 (text/html)]
<div dir="ltr">Hi all,<div><br></div><div style>I've just learned that \
there's a bug number that's exactly what I was trying to fix with this \
patch:</div><div style>Bug 8011888: <a \
href="http://bugs.sun.com/view_bug.do?bug_id=8011888">http://bugs.sun.com/view_bug.do?bug_id=8011888</a></div>
<div style><br></div><div style>If there's already a fix to this bug then \
I'm fine with it. Just like to see the fix in mainline tip so that CLHSDB would \
work out of the box again.</div><div style><br></div><div style> Thanks,</div><div \
style>Kris</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On \
Mon, Jul 29, 2013 at 3:53 PM, Krystal Mok <span dir="ltr"><<a \
href="mailto:rednaxelafx@gmail.com" \
target="_blank">rednaxelafx@gmail.com</a>></span> wrote:<br> <blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr">Hi all,<div><br></div><div>Could I have a \
couple of reviews for this patch, please?</div><div><br></div> <div><a \
href="https://gist.github.com/rednaxelafx/6102608" \
target="_blank">https://gist.github.com/rednaxelafx/6102608</a><br>
</div><div><br></div><div>Switching the JavaScript engine from Rhino to Nashorn and \
the No PermGen project caused a few issues that stopped sa.js from working. This is a \
patch that tries to fix the issues that I've run into.</div>
<div><br></div><div>I'll walkthrough the patch below: (line numbers refer to \
sajs.patch, not sa.js)</div><div><br></div><div>Line \
8-12:</div><div><br></div><div>Since Nashorn implements ECMAScript 5.1, using \
JavaScript keywords as the property name in the "dot" syntax is not a \
problem anymore. So I'm un-commenting these lines.</div>
<div><br></div><div>Line 20-42, 51-76:</div><div><br></div><div>When implementing a \
JavaAdapter for SA ScriptObject, the __get__ function calls the __has__ \
function:</div><div> this.__has__(name)</div> <div>which is working at the wrong \
level: __has__ is a special hook function, and cannot be called via "this" \
this way. It'll trigger the __call__ hook to find the "__has__" member, \
but the JavaAdapter here does not override __call__, and then the lookup will \
fail.</div>
<div><br></div><div>Rather than going through the trouble of implementing the \
__call__ hook just for this purpose, I move the __has__ function up, and made __get__ \
call __has__ directly instead. Now it won't trigger the __call__ hook for the \
lookup, the things will work fine again.</div>
<div><br></div><div>Line 45-48:</div><div><br></div><div>Removed trailing \
whitespace.</div><div><br></div><div>Line 84-85:</div><div><br></div><div>This code \
used to work in Rhino, but apparently Nashorn doesn't automatically convert a \
JavaScript Array into a Java array when doing JS-to-Java interop, so this conversion \
has to be done manually.</div>
<div><br></div><div>Line 93-104:</div><div><br></div><div>I'm a little confused \
in this part. Nashorn exposes a "print" function that's defined in \
jdk/nashorn/api/scripting/resources/engine.js, and that it doesn't expose a \
corresponding "println" function. I'm not sure if this code really \
worked when using Rhino...anyway, the "println" function is missing, so \
"writeln = println" doesn't do anything useful. I'm adding a shim \
here just in case either of "print" or "println" functions are \
missing. This fixes an error when calling "println" in line 87 of this \
patch.</div>
<div><br></div><div>Line 113:</div><div><br></div><div>This is the same change as \
purposed by Yunda back in April. [1] A missing fix from the NPG \
changes.</div><div><br></div><div>Line 121-122, 129-130:</div> \
<div><br></div><div>When specifying a method overload in Nashorn, the argument syntax \
it takes for inner classes uses "." as the separator between the enclosing \
class name and the inner class name, instead of "$" as in the "binary \
name".</div>
<div><br></div><div>Line 138-144, 152-160:</div><div><br></div><div>Nashorn has \
stricter default behaviors than Rhino when overriding Java methods. Where as Rhino \
defaults to a "do nothing" implementation, Nashorn defaults to throwing \
UnsupportedOperationException for unimplemented methods.</div>
<div><br></div><div>Line 168-172:</div><div><br></div><div>Before the fix, this code \
only replace the first occurrence of the specified special characters, which happens \
to be not enough for newer HotSpot types in C++ templates.</div>
<div><br></div><div>That's all for this patch.</div><div><br></div><div>BTW, \
there is another patches pending review, too: JDK-8011979 \
[2]</div><div><br></div><div>Thanks,</div><div> Kris</div><div><br></div><div>[1]: <a \
href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html" \
target="_blank">http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009043.html</a></div>
<div>[2]: <a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html" \
target="_blank">http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-April/009149.html</a></div></div>
</blockquote></div><br></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic