Re: Fwd: Rhino, Changes to JavaMembers.java
Igor Bukanov
> What would be the best way of submitting
> such modifications, btw? The netscape.public.mozilla.jseng newsgroup?
You can either send the patch to the newsgroup (I cross-post the mail
there) or file an enhancement bug at bugzilla.mozilla.org against Rhino.
And please use patches, not the files itself, as it is easier to see
what happens.
>
> I have another idea for modification, this time to gain speed: I noticed
> that the enumeration through all enumratable properties of an object
> works by using getIds(), which creates an array containing all the ids
> and simply iterates through them. Wouldn't it make more sense to use a
> mechanism similar to java's iterators? Like that, no additional arrays
> would have to be created each time. Also, iterating through objects like
> Sets (e.g. LinkedHashSet) would then be possible, as these objects only
> allow access through an iterator, and not by id at all.
>
> I just came acros this problem while trying to wrap a LinkedHashSet in a
> JS object and noticed there may be room for improvement. I checked the
> usage of getIds and noticed it wouldn't bee too much of a change, there
> are 14 usages in the whole source base. getIds in the ScriptableObject
> could still be kept but the default implementation could use the
> iterator to create an array with the ids.
The problem is that any optimization-only changes to Scriptable
interface or any other public API in Rhino are hard to justify as it
would break too many existing embeddings. To demonstrate how bad the
situation could be consider the change in the debug implementation
internals that was done in Rhino 1.5R4 in 2002. That change made Rhino
incompatible with BSF library which explicitly depended on the modified
classes. Since BSF is used in many projects from apache.org and since
the last public release of the library was at the end of 2002, many
people are still forced to use 1.5R3 release of Rhino.
Regards, Igor
Jürg Lehni
> You can either send the patch to the newsgroup (I cross-post the mail
> there) or file an enhancement bug at bugzilla.mozilla.org against
> Rhino. And please use patches, not the files itself, as it is easier to
> see what happens.
Ok, so should I submit again, as a patch? And what do you think of the
proposed changes?
> The problem is that any optimization-only changes to Scriptable
> interface or any other public API in Rhino are hard to justify as it
> would break too many existing embeddings. To demonstrate how bad the
> situation could be consider the change in the debug implementation
> internals that was done in Rhino 1.5R4 in 2002. That change made Rhino
> incompatible with BSF library which explicitly depended on the modified
> classes. Since BSF is used in many projects from apache.org and since
> the last public release of the library was at the end of 2002, many
> people are still forced to use 1.5R3 release of Rhino.
I see. But as getIds() would be kept, I cannot see how this would break
anything at all. I don't know how big the speed gain would be, but to
me it seems a bit crazy to create an array containing all indexes of
the array to be enumerated just to walk through its elements. While i
really like the "for (i in array)" syntax, the fact that Rhino handles
it in that way makes it very slow compared to "for (var i = 0; i <
array.length; i++)" for huge arrays... I just thought that would be an
easy to implement modification that wouldn't really harm any project
depending on this. It could be done by creating a new interface called
'enumeratable', and if the class implements this, the new mechanism is
used. If it's not there, the fallback scenario is using getIds() just
like it was done until now. I could even work on this myself if you
think this makes sense.
Regards,
Jürg
Juerg Lehni
J
Index: JavaMembers.java
===================================================================
RCS file:
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/JavaMembers.java,v
retrieving revision 1.53
diff -r1.53 JavaMembers.java
43a44
> import java.util.Vector;
93,94c94
< Object rval;
< Class type;
---
> Scriptable globalScope = ScriptableObject.getTopLevelScope(scope);
98,99c98
< rval = bp.getter.invoke(javaObject, Context.emptyArgs);
< type = bp.getter.method().getReturnType();
---
> return bp.getter.call(cx, globalScope, scope,
Context.emptyArgs);
102,103c101,104
< rval = field.get(isStatic ? null : javaObject);
< type = field.getType();
---
> Object rval = field.get(isStatic ? null : javaObject);
> Class type = field.getType();
> // Need to wrap the object before we return it.
> return cx.getWrapFactory().wrap(cx, globalScope, rval, type);
108,110d108
< // Need to wrap the object before we return it.
< scope = ScriptableObject.getTopLevelScope(scope);
< return cx.getWrapFactory().wrap(cx, scope, rval, type);
135,141c133,136
< Class setType = bp.setter.argTypes[0];
< Object[] args = { Context.jsToJava(value, setType) };
< try {
< bp.setter.invoke(javaObject, args);
< } catch (Exception ex) {
< throw Context.throwAsScriptRuntimeEx(ex);
< }
---
> NativeJavaMethod setter = (value == null) ? bp.mainSetter
> : bp.setter;
> Object[] args = { value };
> setter.call(Context.getContext(),
ScriptableObject.getTopLevelScope(scope), scope, args);
460a456
>
462,464c458
< // We have a getter. Now, do we have a setter?
< NativeJavaMethod njmSet = null;
< MemberBox setter = null;
---
> // We have a getter. Now, do we have setters?
465a460
> MemberBox[] setters = null;
470,473c465,467
< njmSet = (NativeJavaMethod)member;
< Class type = getter.method().getReturnType();
< setter = extractSetMethod(type,
njmSet.methods,
< isStatic);
---
> NativeJavaMethod njmSet =
(NativeJavaMethod)member;
> Class type = getter.method().getReturnType();
> setters = extractSetMethods(type,
njmSet.methods, isStatic);
477c471
< BeanProperty bp = new BeanProperty(getter, setter);
---
> BeanProperty bp = new BeanProperty(getter, setters);
504,505c498,499
< for (int methodIdx = 0; methodIdx < methods.length; methodIdx++) {
< MemberBox method = methods[methodIdx];
---
> for (int i = 0; i < methods.length; i++) {
> MemberBox method = methods[i];
521c515
< private static MemberBox extractSetMethod(Class type, MemberBox[]
methods,
---
> private static MemberBox[] extractSetMethods(Class type,
MemberBox[] methods,
524,553c518,573
< //
< // Note: it may be preferable to allow
NativeJavaMethod.findFunction()
< // to find the appropriate setter; unfortunately, it
requires an
< // instance of the target arg to determine that.
< //
<
< // Make two passes: one to find a method with direct type assignment,
< // and one to find a widening conversion.
< for (int pass = 1; pass <= 2; ++pass) {
< for (int i = 0; i < methods.length; ++i) {
< MemberBox method = methods[i];
< if (!isStatic || method.isStatic()) {
< if (method.method().getReturnType() == Void.TYPE) {
< Class[] params = method.argTypes;
< if (params.length == 1) {
< if (pass == 1) {
< if (params[0] == type) {
< return method;
< }
< } else {
< if (pass != 2) Kit.codeBug();
< if (params[0].isAssignableFrom(type)) {
< return method;
< }
< }
< }
< }
< }
< }
< }
---
> // find all setter methods that take one parameter and have a
return type of void
> // the method at [0] should be the version with the same type as -
or an asignable
> // type from - the getter method and will be used for setting
null-values, which
> // otherwise would be ambigous if there are several methods to
handle a null value
> // (e.g. a setter with the parameter of class Object and one of
class String)
>
> MemberBox mainSetter = null;
> Vector setMethods = new Vector();
>
> // Make two passes: one to find a main setter method with direct
type assignment,
> // and one to find one with a widening conversion.
> for (int pass = 1; pass <= 2; ++pass) {
> for (int i = 0; i < methods.length; ++i) {
> MemberBox method = methods[i];
> if (!isStatic || method.isStatic()) {
> if (method.method().getReturnType() == Void.TYPE) {
> Class[] params = method.argTypes;
> if (params.length == 1) {
> if (mainSetter == null) { // (still) looking for a main setter?
> if (pass == 1) {
> if (params[0] == type) {
> mainSetter = method;
> // just add it to the beginning of the list:
> setMethods.add(0, mainSetter);
> }
> } else {
> if (params[0].isAssignableFrom(type)) {
> mainSetter = method;
> // in pass 2, see wether it wasn't added already as a
normal setter.
> // if so, remove it in order to add it again at [0]
> int pos = setMethods.indexOf(mainSetter);
> if (pos >= 0)
> setMethods.remove(pos);
> setMethods.add(0, mainSetter);
> }
> }
> } else {
> // if we're in pass 1, just add all the suiting setters to the list.
> // one of these may turn out to be the main setter in pass 2,
so in pass 2,
> // the setMethods needs to be checked if the setter was added
already (see above).
> setMethods.add(method);
> }
> }
> }
> }
> }
> // if a main setter was found, no second pass is needed:
> if (mainSetter != null)
> break;
> }
>
> if (mainSetter != null) {
> MemberBox[] setters = new MemberBox[setMethods.size()];
> setMethods.toArray(setters);
> return setters;
> }
636c656
< BeanProperty(MemberBox getter, MemberBox setter)
---
> BeanProperty(MemberBox getter, MemberBox[] setters)
638,643c658,678
< this.getter = getter;
< this.setter = setter;
< }
<
< MemberBox getter;
< MemberBox setter;
---
> this.getter = new NativeJavaMethod(getter, getter.getName());
>
> if (setters != null) {
> setter = new NativeJavaMethod(setters);
> // if there's more than one setter, setters[0] is the main setter,
> // otherwise it's the same as setter:
> if (setters.length > 1) {
> mainSetter = new NativeJavaMethod(setters[0], setters[0].getName());
> } else {
> mainSetter = setter;
> }
> } else {
> setter = mainSetter = null;
> }
> }
>
> NativeJavaMethod getter;
> NativeJavaMethod setter;
> // main setter is the function with the same parameter type as the
getter's return type.
> // it is used for setting null values.
> NativeJavaMethod mainSetter;
Igor Bukanov
> On 2005-02-16 10:39:01 +0100, Igor Bukanov <ig...@mir2.org> said:
>
>> You can either send the patch to the newsgroup (I cross-post the mail
>> there) or file an enhancement bug at bugzilla.mozilla.org against
>> Rhino. And please use patches, not the files itself, as it is easier
>> to see what happens.
>
>
> Ok, so should I submit again, as a patch?
For a change like that it is better to file a bugzilla.mozilla.org and
attach path (in unified format) there.
> And what do you think of the
> proposed changes?
It is a reasonable proposal.
>
>> The problem is that any optimization-only changes to Scriptable
>> interface or any other public API in Rhino are hard to justify as it
>> would break too many existing embeddings. To demonstrate how bad the
>> situation could be consider the change in the debug implementation
>> internals that was done in Rhino 1.5R4 in 2002. That change made Rhino
>> incompatible with BSF library which explicitly depended on the
>> modified classes. Since BSF is used in many projects from apache.org
>> and since the last public release of the library was at the end of
>> 2002, many people are still forced to use 1.5R3 release of Rhino.
>
>
> I see. But as getIds() would be kept, I cannot see how this would break
> anything at all. I don't know how big the speed gain would be, but to me
> it seems a bit crazy to create an array containing all indexes of the
> array to be enumerated just to walk through its elements. While i really
> like the "for (i in array)" syntax, the fact that Rhino handles it in
> that way makes it very slow compared to "for (var i = 0; i <
> array.length; i++)" for huge arrays...
Do you have any benchmarks that shows that
for (var i = 0; i < array.length; i++) is faster for a huge array?
>I just thought that would be an
> easy to implement modification that wouldn't really harm any project
> depending on this. It could be done by creating a new interface called
> 'enumeratable', and if the class implements this, the new mechanism is
> used. If it's not there, the fallback scenario is using getIds() just
> like it was done until now.
It would not work like that since getIds is used by external
applications. So you would need to implement getIds in terms of the new
interface as well. So it is a lot of code just to optimize not that
often used construction. Plus again, do you really think it would
optimize the enumeration by any noticeable margin?
Regards, Igor
Jürg Lehni
> Jürg Lehni wrote:
>> On 2005-02-16 10:39:01 +0100, Igor Bukanov <ig...@mir2.org> said:
>>
>>> You can either send the patch to the newsgroup (I cross-post the mail
>>> there) or file an enhancement bug at bugzilla.mozilla.org against
>>> Rhino. And please use patches, not the files itself, as it is easier to
>>> see what happens.
>>
>>
>> Ok, so should I submit again, as a patch?
>
> For a change like that it is better to file a bugzilla.mozilla.org and
> attach path (in unified format) there.
Ok, I've submited it now:
https://bugzilla.mozilla.org/show_bug.cgi?id=282595
>> And what do you think of the proposed changes?
>
> It is a reasonable proposal.
>
>>
>>> The problem is that any optimization-only changes to Scriptable
>>> interface or any other public API in Rhino are hard to justify as it
>>> would break too many existing embeddings. To demonstrate how bad the
>>> situation could be consider the change in the debug implementation
>>> internals that was done in Rhino 1.5R4 in 2002. That change made Rhino
>>> incompatible with BSF library which explicitly depended on the modified
>>> classes. Since BSF is used in many projects from apache.org and since
>>> the last public release of the library was at the end of 2002, many
>>> people are still forced to use 1.5R3 release of Rhino.
>>
>>
>> I see. But as getIds() would be kept, I cannot see how this would break
>> anything at all. I don't know how big the speed gain would be, but to
>> me it seems a bit crazy to create an array containing all indexes of
>> the array to be enumerated just to walk through its elements. While i
>> really like the "for (i in array)" syntax, the fact that Rhino handles
>> it in that way makes it very slow compared to "for (var i = 0; i <
>> array.length; i++)" for huge arrays...
>
> Do you have any benchmarks that shows that
> for (var i = 0; i < array.length; i++) is faster for a huge array?
Not really. It's just a guess that it does not really make sense to
create one array of the same length to just walk through another array.
And it's not really a nice design. Iterators would have made much more
sense there.
>
>> I just thought that would be an easy to implement modification that
>> wouldn't really harm any project depending on this. It could be done by
>> creating a new interface called 'enumeratable', and if the class
>> implements this, the new mechanism is used. If it's not there, the
>> fallback scenario is using getIds() just like it was done until now.
>
> It would not work like that since getIds is used by external
> applications. So you would need to implement getIds in terms of the new
> interface as well. So it is a lot of code just to optimize not that
> often used construction. Plus again, do you really think it would
> optimize the enumeration by any noticeable margin?
Yes, but getIds would still be there. Maybe there's a base class
instead of an interface that defines the getIds that relies on the
iterator.
I think the optimization should be quite noticable, especially memory
wise, for bigger arrays...
Jürg
Brendan Eich
> I think the optimization should be quite noticable, especially memory
> wise, for bigger arrays...
Measure twice, cut once.
Premature optimization is the root of all evil (Djikstra?).
/be
Attila Szegedi
> Premature optimization is the root of all evil (Djikstra?).
Nah, it was C. A. R. Hoare. The quote is also often misattributed to Knuth
as he quoted Hoare in one of his papers, "Errors of TeX".
Attila.
--
home: http://www.szegedi.org
weblog: http://www.jroller.com/page/aszegedi
Visit Szegedi Butterfly fractals at:
http://www.szegedi.org/fractals/butterfly/index.html
Attila Szegedi
>
> It would not work like that since getIds is used by external
> applications. So you would need to implement getIds in terms of the new
> interface as well.
Or rather provide a default implementation of the new method (Iterator
iterator()?) in terms of getIds(). Arrays and similar objects could
override iterator() to return a more efficient implementation. BTW, where
I see this better than generating an array of IDs up front - what getId()
does - is
- memory footprint: you need to instantiate one ID at a time instead of
100 000 for a 100 000 element list
- break statements. If the for() breaks in the 40th element of a 100 000
list, you only had to generate 40 numeric IDs.
> So it is a lot of code just to optimize not that often used
> construction. Plus again, do you really think it would optimize the
> enumeration by any noticeable margin?
>
In time, no - you need to create the same number of ID objects. The
additional overhead of AASTORE instructions to populate the array with
those ID objects is probably negligable :-) Actually, there is a time
advantage if the for loop is break-ed, as then the iterator will create
less object.
In memory burden, yes -- only one ID object needs to exist at any one time.
I'd say it's worth a RFE in Bugzilla if the original poster really wants
it :-)
Attila.
Brendan Eich
> On Thu, 17 Feb 2005 11:37:31 -0800, Brendan Eich <bre...@meer.net> wrote:
>
>> Premature optimization is the root of all evil (Djikstra?).
>
>
> Nah, it was C. A. R. Hoare. The quote is also often misattributed to
> Knuth as he quoted Hoare in one of his papers, "Errors of TeX".
Thanks, I remember now.
Where have all the CS greats gone? Knuth is writing letters by hand
now, I hear, and shunning email as a horrible time sink he can't afford
in what time he has left.
/be
Igor Bukanov
...
> I'd say it's worth a RFE in Bugzilla if the original poster really
> wants it :-)
Changing getds would not improve performance of for-in loops since to
follow ECMAScript standard Rhino has to convert indexes to strings.
ECMAScript requires that:
var array = [1];
var type;
for (var i in array) {
type = typeof i;
}
would set type to "string", see ECMA-262 v3, 12.6.4. Theoretically it
should be possible to optimize many common cases to avoid that string
creation but that is not trivial at all. And given that string
conversion changing getIds would not help at all.
Regards, Igor