Improve chances for inlining of ExceptionState::throwIfNeeded()

Improve chances for inlining of ExceptionState::throwIfNeeded()

Since throwIfNeeded() is called quite often in generated bindings code
(and should generally return false), attempt to make is smaller by moving
the exception setup/throwing to a new method, and leave throwIfNeeded()
as a predicate guarding a call to that method.
This seems to get rid of a large number of direct calls to throwIfNeeded().
It also shrinks the binary by a non-negligible amount (~15k).
(Linux, x86-64, gcc-4.8.2)

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170447

[Mike West, 2014-03-31 12:46:40]

LGTM!

[haraken, 2014-03-31 12:49:16]

LGTM.

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:84: ASSERT(!m_exception.isEmpty() ||
m_code);

Can this simply be ASSERT(!m_exception.isEmpty()) ?

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:87: ASSERT(!m_exception.isEmpty());

Then you can remove this.

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:89: if (LIKELY(!hadException()))

Does this LIKELY matter for performance? If you're not sure, you can remove it.
We don't want to add meaningless LIKELY/UNLIKELY :)

[fs, 2014-03-31 12:57:00]

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:84: ASSERT(!m_exception.isEmpty() ||
m_code);
On 2014/03/31 12:49:16, haraken wrote:
> 
> Can this simply be ASSERT(!m_exception.isEmpty()) ?

AFAIU that would then miss the "m_exception.isEmpty() && m_code" case which
existed previously (and is indicated unlikely to ever occur by the FIXME just
below.)
I'd happily remove it but then I suppose the following two lines
(throwDOMException(...)) should also be removed?

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:89: if (LIKELY(!hadException()))
On 2014/03/31 12:49:16, haraken wrote:
> 
> Does this LIKELY matter for performance? If you're not sure, you can remove
it.
> We don't want to add meaningless LIKELY/UNLIKELY :)

TBH I haven't checked generated code without it - I suspect it won't matter much
though (I'll check though). I'll drop unless I find something out that suggest
otherwise.

[haraken, 2014-03-31 13:00:05]

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.cpp (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.cpp:84: ASSERT(!m_exception.isEmpty() ||
m_code);
On 2014/03/31 12:57:00, fs wrote:
> On 2014/03/31 12:49:16, haraken wrote:
> > 
> > Can this simply be ASSERT(!m_exception.isEmpty()) ?
> 
> AFAIU that would then miss the "m_exception.isEmpty() && m_code" case which
> existed previously (and is indicated unlikely to ever occur by the FIXME just
> below.)
> I'd happily remove it but then I suppose the following two lines
> (throwDOMException(...)) should also be removed?

I'd just add ASSERT(!m_exception.isEmpty()) and remove the below if branch, but
you can try that in a separate CL.

[fs, 2014-03-31 13:09:20]

On 2014/03/31 13:00:05, haraken wrote:
>
https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
> File Source/bindings/v8/ExceptionState.cpp (right):
> 
>
https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
> Source/bindings/v8/ExceptionState.cpp:84: ASSERT(!m_exception.isEmpty() ||
> m_code);
> On 2014/03/31 12:57:00, fs wrote:
> > On 2014/03/31 12:49:16, haraken wrote:
> > > 
> > > Can this simply be ASSERT(!m_exception.isEmpty()) ?
> > 
> > AFAIU that would then miss the "m_exception.isEmpty() && m_code" case which
> > existed previously (and is indicated unlikely to ever occur by the FIXME
just
> > below.)
> > I'd happily remove it but then I suppose the following two lines
> > (throwDOMException(...)) should also be removed?
> 
> I'd just add ASSERT(!m_exception.isEmpty()) and remove the below if branch,
but
> you can try that in a separate CL.

I'll do that when this one has landed. Thanks!

[fs, 2014-03-31 13:20:30]

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/219263002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:89: if (LIKELY(!hadException()))
On 2014/03/31 12:57:00, fs wrote:
> On 2014/03/31 12:49:16, haraken wrote:
> > 
> > Does this LIKELY matter for performance? If you're not sure, you can remove
> it.
> > We don't want to add meaningless LIKELY/UNLIKELY :)
> 
> TBH I haven't checked generated code without it - I suspect it won't matter
much
> though (I'll check though). I'll drop unless I find something out that suggest
> otherwise.

Dropped.

[fs, 2014-03-31 13:20:34]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-03-31 13:20:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/219263002/20001

[commit-bot: I haz the power, 2014-03-31 14:32:25]

Change committed as 170447