347248 - Optimizing the stack property of error objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

SpiderMonkey supports the stack property on error objects to report the full stack trace about the exception origin. But this support is relatively expensive. Consider, for example, the following benchmark (see also http://celtickane.com/projects/jsspeed.php for a similar benchmark):

function testTryCatch()
{
        var timeFunction = Date.now;
        var i = 5000;
        var time = timeFunction();
        do {
                try { throw new Error(); } catch (e) { }
        } while (--i);
        time = timeFunction() - time;
        return time;
}

var time = testTryCatch();
if (typeof print != "function")
        print = alert;
print("Execution time: "+time+" ms");

The benchmark runs more than 2 times faster with stack initialization from InitExceptionObject in jsexn.c disabled.

This is rather slow and we should try to optimize it so a code that use exceptions, for example, to simulate proper tail calls, http://w3future.com/weblog/2006/02/ would not suffer that much.

One possibility would be not to create a full string during exception construction but rather capture just enough information to construct it later.

Comment 1

[Igor Bukanov]

Attachment: Implementation v1

The patch extends JSExnPrivate to hold enough information to reconstruct the stack trace on the first access to the stack property in the resolve hook.

The patch speed-up the test case by factor 1.8.

I will try to extend the patch to define all exception properties lazily to see if the extra code would justify the speedup.

Comment 2

[Igor Bukanov]

Attachment: Implementation v2

This patch defines all properties of new Error objects lazily. It speeds up the original test case by the factor of 2.5. I did not expected that DefineProperty calls was that expensive.

Comment 3

[Igor Bukanov]

Attachment: Implementation v3

This is effectively the previous implementation without already committed parts to fix bug 349527 and bug 348532. I also tried to minimize the amount of code movements in the patch.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 235921 [details] [diff] [review]
Implementation v3

This is great -- nits only:

1. s/ExcPrivate/ExnPrivate/g or else s/ExnPrivate/ExcPrivate/g -- "exc" is traditional in many languages, but jsexn.c spells it "exn" and has since mccabe wrote it, so I favor local tradition.

2. JSSTRING_MAX_LENGTH should be JSSTRING_LENGTH_MAX, trailing _MAX convention is local and Unix <limits.h> tradition, matches other suffixes.

Do you think we should get this into js1.7/moz1.8.1?  Time is short.  In any event I'll stamp a nit-picked patch for the trunk, to get it baking.

With this and lexical catch vars, how is our try/catch synthetic benchmark performance (http://celtickane.com/projects/jsspeed.php) now?

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 235921 [details] [diff] [review]
Implementation v3

Few more nits:

>+static jsval *
>+GetStackTraceValueBuffer(JSExnPrivate *priv)
>+{
>+    /*
>+     * We use extra memory after JSExnPrivateInfo.stackElems to store jsvals
>+     * that helps to produce more informative stack traces. The following
>+     * assert allows to assume that no gap after stackElems is necessary to

"allows us to assume ..."


>+    /*
>+     * We initialize errorReport with a copy of report after setting the
>+     * private slot so to prevent GC accessing a junk value we clear the field

"so as to prevent", or just "... private slot, to prevent"

>+        /*
>+         * Construct a new copy of the error report struct. We can't use the
>+         * error report struct that was passed in, because it's
>+         * stack-allocated, and also because it may point to transient data in
>+         * the JSTokenStream.

Pre-existing comment (IIRC) but maybe wrap it more nicely by writing "it's allocated on the stack, and ...."

/be

Comment 6

[Igor Bukanov]

Attachment: Implementation v4

The new version addresses the nits and removes changes from jsstr.h, which was a leftover from an initial prototype.

Note that the patch continues to define fileName, lineNumber and message as properties if Error constructor is uses with object that does not have js_ErrorClass. The question is this possible especially given that Error() as function is equivalent to new Error() ?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Note that the patch continues to define fileName, lineNumber and message as
> properties if Error constructor is uses with object that does not have
> js_ErrorClass. The question is this possible especially given that Error() as
> function is equivalent to new Error() ?

obj = {}; Error.call(obj, "foo")

doesn't do it, because the Error constructor always returns a new object of Error class:

js> obj = {}
[object Object]
js> obj2 = Error.call(obj, "foo")
Error: foo
js> obj2 === obj
false
js> obj2.name
Error
js> obj2.message
foo

So it looks like there's no point in supporting vanilla objects on that account.  But what if one does

js> obj3 = {__proto__:Error.prototype}
Error
js> obj3.name 
Error
js> obj3.message = "bar"
bar
js> obj3.message
bar

Still, this does not require InitExceptionObject unless it's needed for Error.prototype initialization.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 236293 [details] [diff] [review]
Implementation v4

r=me, looking good -- if you can get rid of over-general InitExceptionObject, so much the better -- I'll restamp that patch.

/be

Comment 9

[Igor Bukanov]

Attachment: Implementation v5

This is v4 with InitExceptionObject removed and JSCLASS_NEW_RESOLVE added (it was lost after I "accounted" for changes in v3, see comment 3)

Comment 10

[Igor Bukanov]

Stats from http://celtickane.com/projects/jsspeed.php on my Pentium M 1.1GHz laptom with Ubuntu 6.06 (the best from 5 runs):

340: Firefox 1.5.0.6
303: Optimized trunk build without the patch
161: Optimized trunk build with the patch
16:   Opera 9.01 for Linux (yes, 16!)

Comment 11

[Igor Bukanov]

Attachment: Implementation v6

Important fix compared with v5: checkAccess in InitExnPrivate must be called with valid target value pointer. I.e. that /* ignored */ referred to the use of v after the call, not before.

Comment 12

[Igor Bukanov]

Attachment: Implementation v6 for real

Please ignore the previous patch: it was from another bug :(

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 236465 [details] [diff] [review]
Implementation v6 for real

Thanks, still good (sorry I missed the dropped JSCLASS_NEW_RESOLVE -- I even got the bugzilla interdiff working but missed it).

/be

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Stats from http://celtickane.com/projects/jsspeed.php on my Pentium M 1.1GHz
> laptom with Ubuntu 6.06 (the best from 5 runs):
> 
> 340: Firefox 1.5.0.6
> 303: Optimized trunk build without the patch
> 161: Optimized trunk build with the patch

Interesting -- so lexical catch vars shaved ~11% off of the 1.5.0.6 time, but this patch shaved more like 47% off the remaining critical path length.

> 16:   Opera 9.01 for Linux (yes, 16!)

We should aim to beat this time!

Igor, you didn't comment on whether to nominate this bug's fix for 1.8.1 approval. I'd like to get it in for not only performance, but code hygiene, but you should weigh the pros and cons for drivers to evaluate.  Thanks,

/be

Comment 15

[Igor Bukanov]

Attachment: Implementation v7

I got an assert with v6. Apparently it can be fp->fun != null and fp->argv == null. The patch fixes this.

Comment 16

[Igor Bukanov]

Attachment: HTML test for performance testing

Comment 17

[Igor Bukanov]

Attachment: Better HTML performance test case

A better test case to get results that do not depend on GC running at arbitrary moments.

Comment 18

[Igor Bukanov]

Here is the stats for the above test case for my 1.1 GHz laptop with Ubuntu 6.06:

1.5  T   TP   O

92   80  46   4   try { throw new Error() } catch {}
51   20  20   6   try { throw new scriptFunction() } catch {}
108 96  67  11  try { throw new Error() } catch (e){ access e }
62   30  29  13  try { throw new scriptFunction() } catch (e) { access e }


1.5: Firefox 1.5.0.6
T:   Trunk
TP:  Trunk with the patch
O:   Opera 9.01 For Linux

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 236474 [details] [diff] [review]
Implementation v7

Hmm, not sure what other than the compiler could set fp->fun but not fp->argv -- then it must have been the compiler.  Any way to confirm or test that it was a compile-time error as exception whose stack property was being computed?

/be

Comment 20

[Igor Bukanov]

I committed the patch from comment 15 to the trunk:

Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.86; previous revision: 3.85
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.56; previous revision: 3.55
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.75; previous revision: 3.74
done

Comment 21

[Igor Bukanov]

(In reply to comment #14)
> Igor, you didn't comment on whether to nominate this bug's fix for 1.8.1
> approval. I'd like to get it in for not only performance, but code hygiene, but
> you should weigh the pros and cons for drivers to evaluate.  Thanks,

Let it bake on the trunk for few days. If there would be a regression, then no branch nominations from me. If everything is OK, I would ask for approval. 

But what this patch shows is how difficult to add even lazily initialized properties to the object. The amount of code is just too high. It would be nice to have a simple API to say that a particular property should be permanently mapped to a particular slot. With such API all the efforts to define enumerate and resolve hook would be unnecessary.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> But what this patch shows is how difficult to add even lazily initialized
> properties to the object. The amount of code is just too high. It would be nice
> to have a simple API to say that a particular property should be permanently
> mapped to a particular slot. With such API all the efforts to define enumerate
> and resolve hook would be unnecessary.

Yes, the property-oriented API is eleven years old and it hasn't evolved at all. It should be easy to add generic lazy resolve/enumerate functions that could be used with the right initialized data structures.  Or something like that -- want to file a bug?

/be

Comment 23

[Igor Bukanov]

I reopen this to fix old GCC support.

Comment 24

[Igor Bukanov]

I took away the patch:

Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.87; previous revision: 3.86
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.57; previous revision: 3.56
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.76; previous revision: 3.75
done

Comment 25

[Igor Bukanov]

Attachment: Implementation v8

This is a version with just compiler-time constants in the initializer.

Comment 26

[Igor Bukanov]

Attachment: Implementation v9

The previous patch had too tight static assert.

Comment 27

[Igor Bukanov]

(In reply to comment #22)
> Yes, the property-oriented API is eleven years old and it hasn't evolved at
> all. It should be easy to add generic lazy resolve/enumerate functions that
> could be used with the right initialized data structures.  Or something like
> that -- want to file a bug?

See bug 351186. 


> 
> /be
> 

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 236557 [details] [diff] [review]
Implementation v9

r=me yet again.

/be

Comment 29

[Igor Bukanov]

I committed the patch from comment 26 to the trunk:

Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.88; previous revision: 3.87
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.58; previous revision: 3.57
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.77; previous revision: 3.76
done

Comment 30

[Igor Bukanov]

This is an optimization that was on the trunk for sufficient time to justify its inclusion to 1.8.1.1 and JS1.7.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 236557 [details] [diff] [review]
Implementation v9

approved for 1.8 branch, a=dveditz for drivers

Comment 32

[Igor Bukanov]

I committed the patch from comment 26 to MOZILLA_1_8_BRANCH:

Checking in jsatom.c;
/cvsroot/mozilla/js/src/jsatom.c,v  <--  jsatom.c
new revision: 3.65.4.5; previous revision: 3.65.4.4
done
Checking in jsatom.h;
/cvsroot/mozilla/js/src/jsatom.h,v  <--  jsatom.h
new revision: 3.40.4.5; previous revision: 3.40.4.4
done
Checking in jsexn.c;
/cvsroot/mozilla/js/src/jsexn.c,v  <--  jsexn.c
new revision: 3.50.4.10; previous revision: 3.50.4.9
done

Comment 33

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1.1, 1.9 windows/linux