I went ahead and rewrote the code to use a newly introduced PropertyDescriptor class.
You can look at the code on my forked Rhino repository on Github: http://github.com/lehni/rhino/commits/property-descriptor http://github.com/lehni/rhino/commit/827cfa26bea4fda4b8f170b50beb74c99f692b28 The patch looks longer than it really is, many of the changes come from fixing the formatting issues at the same time... I did some performance tests and saw an improvement of around 50%. Ss another side effect, the code that uses defineOwnPropery / getOwnPropertyDescriptor internally became shorter, faster and more readable. Most of the PropertyDescriptor related functionality was moved to the new class, removing complexity from the already long ScriptableObject class. There is now only one public defineOwnProperty() method on ScriptableObject that takes a PropertyDescriptor object as an argument and can be overridden in other classes to register property changes. Looking at this now, I think getOwnPropertyDescriptor() should be made public too, it is still protected right now. Please note that PropertyDescriptor only serves as an internal caching class, it is never returned to the scripting side of things. And here some of the optimisations: - The GetterSlot no longer needs to modify the NativeObject returned by Slot to turn it from a DataDescriptor into an AccessorDescriptor. It now can simply use the ProperyDescriptor constructor that takes a getter and a setter object to directly create an object that represents a AccessorDescriptor. - The use of SLOT_MODIFY_GETTER_SETTER for modifying accessor slots in defineOwnProperty() directly when making a new slot, in order to avoid conversion from a normal slot to a accessor slot later on. - checkValidPropertyDescriptor() calls slot.getPropertyDescriptor() directly instead of passing through getOwnPropertyDescriptor() again. - The scope used to create NativeObjects does not need to be determined and passed on in many places, as the conversion to a NativeObject only happens at the end, leading to many simplifications and simpler code. How shall we proceed from here? Norris, how do you feel about this proposal? Jürg On 20 Apr 2010, at 15:12, Rapha wrote: > Hi, > > I wrote most of Object.defineProperty, so I guess I can answer to some > of these points. > > On Apr 19, 8:00 pm, Jürg Lehni <[email protected]> wrote: >> I started experimenting with the various Object.defineProperty related >> functions in Rhino and found a few issues that I am happy to correct, but >> would like to start a discussion first before I write a patch: >> >> First of all, all the code from that patch is using different formatting >> rules than the rest of Rhino: Two white spaces instead of four for >> intention, no limit to line length, so it would be good to also correct this >> along the other things discussed here. >> > > Yes, feel free to correct that. I guess I was using the wrong editor > setting when I wrote that code. > >> Beyond these cosmetic changes, here a list of what I found and would suggest >> to change / improve: >> >> - There seem to be a row of issues with checkValidPropertyDefinition() not >> acting according to the specs. One I found is that if configurable is false >> for a property that was defined by a data descriptor, I can still convert it >> to a getter / setter once (and then probably back to a normal data >> property). But I cannot change the getter again after it was already defined >> as a getter. On the following page it is stated: "If the old descriptor had >> its configurable field as false, then no fields present in the new >> descriptor can be different from those in the old descriptor. Further, the >> type of the new descriptor must be the same as the type of the old one." So >> it seems this behaviour should be fixed. >> >> https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global... >> > > Perhaps I'm misunderstanding you, but I can't replicate the behaviour > you describe with: > "if configurable is false for a property that was defined by a data > descriptor, I can still convert it to a getter / setter once " > > I get: > [rhino(master)]$ java -jar build/rhino1_7R3pre/js.jar -version 180 > Rhino 1.7 release 3 PRERELEASE 2010 01 06 > js> var obj = {a:1} > js> Object.defineProperty(obj, "b", {configurable:false, > enumerable:true, value:5}) > [object Object] > js> Object.defineProperty(obj, "b", {configurable:false, > enumerable:true, get: function(){print("gotcha"); return 6}}) > js: "<stdin>", line 4: uncaught JavaScript runtime exception: > TypeError: Cannot change "b" from a data property to an accessor > property because configurable is false. > at <stdin>:4 > > js> > > Could you write a failing test for this behaviour? > > >> - The various methods checkValidPropertyDefinition(), isDataDescriptor(), >> isAccessorDescriptor(), isGenericDescriptor() repeatedly access properties >> on the passed descriptor ScriptableObject through >> ScriptableObject.hasProperty() / ScriptableObject.getProperty(), so >> traveling up the inheritance chain each time. None of this is cached >> anywhere, and methods like isAccessorDescriptor() etc. are used in many >> places, each time accessing these values again. I see the potential for a >> performance improvement by introducing a new class (e.g. PropertyDescriptor) >> that holds all the properties of the descriptors, including information >> about its state / type, and some logic to convert back and forth from >> ScriptableObjects and attributes flags. >> >> - GetterSlot#getPropertyDescriptor() erases properties again that were set >> by Slot#getPropertyDescriptor(). This does not seem optimal. If the code was >> refactured to return a PropertyDescriptor object instead which then can >> convert itself to a ScriptableObject, this could be solved elegantly. >> > > I agree that the PropertyDescriptor class sounds like a better > solution. However when I initially started working on defineProperty, > the feedback was that the this abstraction was not a good idea, so > best to confirm with Norris et al about that one. > > See https://bugzilla.mozilla.org/show_bug.cgi?id=489329#c3 > > >> I am curious to hear your thoughts on this matter, and am happy to commit >> some time to this. >> >> Jürg > > Cheers, > Raphael > _______________________________________________ > dev-tech-js-engine-rhino mailing list > [email protected] > https://lists.mozilla.org/listinfo/dev-tech-js-engine-rhino _______________________________________________ dev-tech-js-engine-rhino mailing list [email protected] https://lists.mozilla.org/listinfo/dev-tech-js-engine-rhino
