573792 - Narcissus JS: need fallback for when __applyConstructor__ is not available.

Status: RESOLVED FIXED

(Other Applications Graveyard :: Narcissus, defect)

Description

[Tom Austin]

Attachment: Patch to allow Narcissus to work when __applyConstructor__ is not available.

Since __applyConstructor__ is only available in special builds, we need a fallback solution.

The attached patch will work for instances of up to 9 arguments to a constructor, which will pass the bulk of the jstests.py unit tests:

  $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt 
  [1156|   6| 150] 100% ===============================================>|  439.3s
  FIXES
      narcissus/../js1_8_5/regress/regress-555246-1.js
  REGRESSIONS
      narcissus/../ecma/Array/15.4.2.1-2.js
      narcissus/../ecma/Array/15.4.2.1-3.js
      narcissus/../ecma/Array/15.4.4.4-1.js
      narcissus/../ecma/Array/15.4.5.1-1.js
      narcissus/../ecma/Array/15.4.5.2-2.js
      narcissus/../ecma_3/extensions/regress-103087.js
  FAIL


With __applyConstructor__, no new tests fail:

  $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt 
  [1162|   0| 150] 100% ===============================================>|  446.1s
  FIXES
      narcissus/../js1_8_5/regress/regress-555246-1.js
  PASS

Comment 1

[Andreas Gal :gal]

Can we eval our way out of this if we have more than N args? Its slow, but it would be a fallback (instead of the PANIC thing).

Comment 2

[Tom Austin]

Attachment: Patch using eval in place of __applyConstructor__

New patch using eval as a fallback for when __applyConstructor__ is not available.  Performance actually seems pretty comparable.

Comment 3

[Andreas Gal :gal]

Comment on attachment 453226 [details] [diff] [review]
Patch using eval in place of __applyConstructor__

>diff -r f0fa8aa7f049 js/narcissus/jsexec.js
>--- a/js/narcissus/jsexec.js	Tue Jun 22 15:37:18 2010 -0700
>+++ b/js/narcissus/jsexec.js	Tue Jun 22 16:09:56 2010 -0700
>@@ -906,17 +906,27 @@ if (!('__call__' in Fp)) {
> 
>     REp.__defineProperty__('__call__', function (t, a, x) {
>         a = Array.prototype.splice.call(a, 0, a.length);
>         return this.exec.apply(this, a);
>     }, true, true, true);
> 
>     Fp.__defineProperty__('__construct__', function (a, x) {
>         a = Array.prototype.splice.call(a, 0, a.length);
>-        return this.__applyConstructor__(a);
>+        if (this.__applyConstructor__){
>+            return this.__applyConstructor__(a);
>+        }

Newline.

I think I would prefer a switch for 0..3 or so and then the eval hack and no mentioning of __applyConstructor__ at all.

>+        // Fallback for when __applyConstructor__ is not available.
>+        else {
>+            var argStr = "";
>+            for (var i in a) {
>+                argStr += 'a[' + i + '],';
>+            }
>+            return eval('new this(' + argStr.slice(0,-1) + ');');
>+        }
>     }, true, true, true);
> 
>     // Since we use native functions such as Date along with host ones such
>     // as global.eval, we want both to be considered instances of the native
>     // Function constructor.
>     Fp.__defineProperty__('__hasInstance__', function (v) {
>         return v instanceof Function || v instanceof global.Function;
>     }, true, true, true);

Comment 4

[Andreas Gal :gal]

r+ with comments means you have to fix the comments before pushing, but its close enough to the final result that you don't have to ask for review again.

Comment 5

[Tom Austin]

Attachment: Updated patch to eliminate appluyConstructor completely

Comment 6

[Andreas Gal :gal]

Comment on attachment 457454 [details] [diff] [review]
Updated patch to eliminate appluyConstructor completely

Maybe loop from 0..a.length-1? That seems clearer.

Comment 7

[Tom Austin]

Changelist: http://hg.mozilla.org/tracemonkey/rev/11aefd0d4f37

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Comment on attachment 457454 [details] [diff] [review]
> Updated patch to eliminate appluyConstructor completely
> 
> Maybe loop from 0..a.length-1? That seems clearer.

That is a good suggestion, since slice is costly even with dependent (prefix) string

Also, please put a space on both sides of binary operators including = and <.

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/11aefd0d4f37