Optimizing the stack property of error objects

VERIFIED FIXED

Status

()

--
enhancement
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({verified1.8.1.1})

Trunk
verified1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 233400 [details] [diff] [review]
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.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 233464 [details] [diff] [review]
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.
Attachment #233400 - Attachment is obsolete: true
(Assignee)

Comment 3

12 years ago
Created attachment 235921 [details] [diff] [review]
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.
Attachment #233464 - Attachment is obsolete: true
Attachment #235921 - Flags: review?(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 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
(Assignee)

Comment 6

12 years ago
Created attachment 236293 [details] [diff] [review]
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() ?
Attachment #235921 - Attachment is obsolete: true
Attachment #236293 - Flags: review?(brendan)
Attachment #235921 - Flags: review?(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 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
Attachment #236293 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

12 years ago
Created attachment 236371 [details] [diff] [review]
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)
Attachment #236293 - Attachment is obsolete: true
Attachment #236371 - Flags: review?(brendan)
(Assignee)

Comment 10

12 years ago
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!)
(Assignee)

Comment 11

12 years ago
Created attachment 236463 [details] [diff] [review]
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.
Attachment #236371 - Attachment is obsolete: true
Attachment #236463 - Flags: review?(brendan)
Attachment #236371 - Flags: review?(brendan)
(Assignee)

Comment 12

12 years ago
Created attachment 236465 [details] [diff] [review]
Implementation v6 for real

Please ignore the previous patch: it was from another bug :(
Attachment #236463 - Attachment is obsolete: true
Attachment #236465 - Flags: review?(brendan)
Attachment #236463 - Flags: review?(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
Attachment #236465 - Flags: review?(brendan) → review+
(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
(Assignee)

Comment 15

12 years ago
Created attachment 236474 [details] [diff] [review]
Implementation v7

I got an assert with v6. Apparently it can be fp->fun != null and fp->argv == null. The patch fixes this.
Attachment #236465 - Attachment is obsolete: true
Attachment #236474 - Flags: review?(brendan)
(Assignee)

Comment 16

12 years ago
Created attachment 236475 [details]
HTML test for performance testing
(Assignee)

Comment 17

12 years ago
Created attachment 236477 [details]
Better HTML performance test case

A better test case to get results that do not depend on GC running at arbitrary moments.
Attachment #236475 - Attachment is obsolete: true
(Assignee)

Comment 18

12 years ago
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 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
Attachment #236474 - Flags: review?(brendan) → review+
(Assignee)

Comment 20

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

12 years ago
(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.
(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
(Assignee)

Comment 23

12 years ago
I reopen this to fix old GCC support.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

12 years ago
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
(Assignee)

Comment 25

12 years ago
Created attachment 236556 [details] [diff] [review]
Implementation v8

This is a version with just compiler-time constants in the initializer.
Attachment #236474 - Attachment is obsolete: true
Attachment #236556 - Flags: review?(brendan)
(Assignee)

Comment 26

12 years ago
Created attachment 236557 [details] [diff] [review]
Implementation v9

The previous patch had too tight static assert.
Attachment #236556 - Attachment is obsolete: true
Attachment #236557 - Flags: review?(brendan)
Attachment #236556 - Flags: review?(brendan)
(Assignee)

Comment 27

12 years ago
(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 on attachment 236557 [details] [diff] [review]
Implementation v9

r=me yet again.

/be
Attachment #236557 - Flags: review?(brendan) → review+
(Assignee)

Comment 29

12 years ago
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
(Assignee)

Updated

12 years ago
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: in-testsuite-
(Assignee)

Comment 30

12 years ago
This is an optimization that was on the trunk for sufficient time to justify its inclusion to 1.8.1.1 and JS1.7.
Blocks: 355044
Flags: blocking1.8.1.1?
(Assignee)

Updated

12 years ago
Attachment #236557 - Flags: approval1.8.1.1?
(Assignee)

Updated

12 years ago
Blocks: 358192
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment on attachment 236557 [details] [diff] [review]
Implementation v9

approved for 1.8 branch, a=dveditz for drivers
Attachment #236557 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 32

12 years ago
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
Keywords: fixed1.8.1.1

Comment 33

12 years ago
verified fixed 1.8.1.1, 1.9 windows/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.1 → verified1.8.1.1
You need to log in before you can comment on or make changes to this bug.