Closed
Bug 347248
Opened 18 years ago
Closed 18 years ago
Optimizing the stack property of error objects
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: verified1.8.1.1)
Attachments
(2 files, 10 obsolete files)
2.15 KB,
text/html
|
Details | |
36.89 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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 4•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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)
Comment 7•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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•18 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•18 years ago
|
||
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•18 years ago
|
||
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 13•18 years ago
|
||
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+
Comment 14•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
Assignee | ||
Comment 17•18 years ago
|
||
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•18 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 19•18 years ago
|
||
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•18 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
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•18 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.
Comment 22•18 years ago
|
||
(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•18 years ago
|
||
I reopen this to fix old GCC support.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•18 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•18 years ago
|
||
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•18 years ago
|
||
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•18 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 28•18 years ago
|
||
Comment on attachment 236557 [details] [diff] [review]
Implementation v9
r=me yet again.
/be
Attachment #236557 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 29•18 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•18 years ago
|
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 30•18 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: js1.7src
Flags: blocking1.8.1.1?
Assignee | ||
Updated•18 years ago
|
Attachment #236557 -
Flags: approval1.8.1.1?
Updated•18 years ago
|
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment 31•18 years ago
|
||
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•18 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•18 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.
Description
•