[webkit-reviews] review denied: [Bug 119362] Save last exceptionStack before saving current exceptionStack. : [Attachment 207889] patch 1

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Chris Curtis
<chris_curtis at apple.com>'s request for review:
Bug 119362: Save last exceptionStack before saving current exceptionStack.
https://bugs.webkit.org/show_bug.cgi?id=119362

Attachment 207889: patch 1
https://bugs.webkit.org/attachment.cgi?id=207889&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207889&action=review


> Source/JavaScriptCore/ChangeLog:10
> +	   1. The vm->exceptionStack is unconditionally updated at each throw,
and error 
> +	       objects are only updated when they do not have the stack
property.

You should mention the 'why' here: This matches other browsers, and Java VMs.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:617
> -	   if (!callFrame->vm().exceptionStack().size()) {
> +	   if (!callFrame->vm().getExceptionStack().size()) {

WebKit style only uses the "get" prefix for functions that have out parameters.
This should be "exceptionStack()".

> Source/JavaScriptCore/runtime/VM.cpp:535
> +void VM::setExceptionStack(const RefCountedArray<StackFrame> newStack)

Passing an object like this will invoke its copy constructor. In the case of
RefCountedArray, that will copy a pointer on the stack and then increment a
refcount. When the temporary copy is destroyed, it will do some branching and
possibly decrement the refcount. This is needless wasted performance.

You should pass "const RefCountedArray<StackFrame>&" to avoid this cost.

> Source/JavaScriptCore/runtime/VM.h:336
> +	   RefCountedArray<StackFrame> getExceptionStack() const { return
m_exceptionStack; } 
> +	   void setExceptionStack(const RefCountedArray<StackFrame>);
> +	   RefCountedArray<StackFrame>& lastExceptionStack() { return
m_lastExceptionStack; }

I believe it's sufficient to have a single "lastExceptionStack". Any new
exception can just overwrite the last lastExceptionStack. Is there any time
when we want to know the current exception stack and the last simultaneously?