476443 - Exception handling may be slow and use a lot of space

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Lars T Hansen]

There are reports (lacking data) that our current exception handling strategy, which uses setjmp + longjmp, may be slow.  We should investigate if it is much slower than the competition, and if so, whether it's worth doing something about it.

(It's credible that it's slow; exception handling can be implemented with very little overhead even for when a handler is set up, whereas setjmp is expensive in many cases.)

Comment 1

[Steven Johnson]

it also uses a bunch of stack space, especially on PowerPC -- alternate approaches might be worthwhile there (even if no speed gain) if they saved stack space.

Comment 2

[Lars T Hansen]

There was a recent thread on LEGO about this, with some numbers.  Adobe devs: contact me about it if you wish.

Comment 3

[Krzysztof Palacz]

On OSX 10 (checked on 10.6) setjmp results in 2 system calls needed to query the OS about signal state (sigprocmask and sigaltstack). sigsetjmp or _setjmp could be used instead to avoid OS calls. Very limited testing suggests that this approach would work. 
However, it's unlikely but not entirely impossible that some player code sets up a code region with a different signal mask and potentially throws an exception from within that region.

Comment 4

[Lars T Hansen]

(In reply to comment #3)
> However, it's unlikely but not entirely impossible that some player code sets
> up a code region with a different signal mask and potentially throws an
> exception from within that region.

We could say "don't do that" and test for it in debug builds, maybe.

Ongoing API work (Michael Daumling / Bernd Mathiske) is looking at making sure that exceptions don't cross the VM / Player boundary, which will be helpful too.

Comment 5

[Krzysztof Palacz]

Attachment: Preliminary patch, intended for testing.

Comment 6

[Lars T Hansen]

The patch works OK in the Player testing I've done so far.

Krzysztof, can you provide a patch with some comments regarding why it's doing what it's doing and ask me or Edwin for a review?  We should land it to see if anything breaks - I don't know what else we can do to test it properly.

Extra points for a DEBUG implementation that checks that signal masks are invariant as we expect them to be.

Comment 7

[Krzysztof Palacz]

Attachment: adds comments

Started working on asserts but not done.

Comment 8

[Lars T Hansen]

Comment on attachment 426008 [details] [diff] [review]
adds comments

Good so far.  Probably "routinve" should be "routine".

Comment 9

[Krzysztof Palacz]

Attachment: Fixes typo

Comment 10

[Krzysztof Palacz]

Attachment: Test only patch with debug asserts

Manually saves the 32 bit sigmask in the ExceptionFrame before setjmp, and verifies when setjmp returns as a result of longjmp, that the saved sigmask is still the same.

Comment 11

[Lars T Hansen]

Comment on attachment 426031 [details] [diff] [review]
Test only patch with debug asserts

Should use 'uint32_t' or similar sized type, not 'unsigned'.

Declaration for save_signal_mask says it returns int but definition says it returns unsigned.

The house style is mostly saveSignalMask() not save_signal_mask().

Signals are not available on all platforms so there needs to be some sort of guard on that.  We should take that conversation to email.  Include Ed and Steven (at least).

Comment 12

[Krzysztof Palacz]

Attachment: Incorporated comments, cleanup

AMVPLUS_MAC is used in PrintWriter but not defined, at least when building with xcode project files.
Using _MAC instead, already used in AvmCore and elsewhere.

Comment 13

[Krzysztof Palacz]

Attachment: typos and cosmetics

Comment 14

[Lars T Hansen]

Comment on attachment 426061 [details] [diff] [review]
typos and cosmetics

You don't need to use _MAC in MacDebugUtils.cpp, it's trivially true there.

AVMPLUS_MAC is indeed defined, in core/avmfeatures.{as,h} and you should use that instead.  (Xcode is a little confused about #defines, IME.)

r+ conditional on those changes.

Comment 15

[Krzysztof Palacz]

Attachment: Incorporated latest comments

Comment 16

[Dan Smith]

Narrowing this bug's focus to setjmp signal handling change.  Stack space investigation separated to Bug 545409 and marked for Future.

Comment 17

[Krzysztof Palacz]

Attachment: Moves signal utilities to PosixPortUtils.cpp

MacDebugUtils.cpp included in all builds.

Comment 18

[Krzysztof Palacz]

Erm, that should read, MacDebugUtils.cpp is *not* included in all builds.

Comment 19

[Lars T Hansen]

(In reply to comment #17)
> Created an attachment (id=426286) [details]
> Moves signal utilities to PosixPortUtils.cpp

Consider this patch approved.

Comment 20

[Steven Johnson]

(In reply to comment #19)
> (In reply to comment #17)
> > Created an attachment (id=426286) [details] [details]
> > Moves signal utilities to PosixPortUtils.cpp
> 
> Consider this patch approved.

Patch does not compile for me (!) on shell64 target...

Comment 21

[Steven Johnson]

(In reply to comment #20)
> Patch does not compile for me (!) on shell64 target...

It appears that <signal.h> needs explicit inclusion for some build configs. I'll add that before landing.

Comment 22

[Steven Johnson]

pushed as http://hg.mozilla.org/tamarin-redux/rev/92213d7b6cc6

do we want to leave this open (to consider a similar linux-specific fix)?

Comment 23

[Lars T Hansen]

(In reply to comment #22)
> pushed as http://hg.mozilla.org/tamarin-redux/rev/92213d7b6cc6
> 
> do we want to leave this open (to consider a similar linux-specific fix)?

It behooves us to at least investigate the issue on Linux.

Comment 24

[Dan Smith]

Yes, let's investigate  at the situation on Linux too.

Comment 25

[Krzysztof Palacz]

Attachment: Equivalent fix for Linux.

Comment 26

[Lars T Hansen]

Comment on attachment 427190 [details] [diff] [review]
Equivalent fix for Linux.

In Exception.cpp the declaration of assertSignalMask and querySignalMask are platform-dependent, but their uses are platform-independent.  This code will fail to compile on some platforms, if I'm not mistaken.

Presumably you'd have to either reintroduce the platform ifdefs around the calls, or make those functions available on all platforms in debug mode.

Comment 27

[Krzysztof Palacz]

Attachment: Incorporated review comments, added JIT fix (affects the Mac as well)

Tentative, seeing significant improvement on the Mac on at least one benchmark.

Comment 28

[Steven Johnson]

Comment on attachment 427221 [details] [diff] [review]
Incorporated review comments, added JIT fix (affects the Mac as well)

shouldn't __linux__ be AVMPLUS_LINUX?

Comment 29

[Lars T Hansen]

(In reply to comment #28)
> (From update of attachment 427221 [details] [diff] [review])
> shouldn't __linux__ be AVMPLUS_LINUX?

That appears not to be defined anywhere.  It is /used/ in one place, though... as is LINUX, which also is not defined anywhere.

:-)

Comment 30

[Lars T Hansen]

Comment on attachment 427221 [details] [diff] [review]
Incorporated review comments, added JIT fix (affects the Mac as well)

Fine with me.  You should ask Edwin for review too since it concerns CodegenLIR.

Comment 31

[Steven Johnson]

Comment on attachment 427221 [details] [diff] [review]
Incorporated review comments, added JIT fix (affects the Mac as well)

http://hg.mozilla.org/tamarin-redux/rev/ee91fa9c95a5

Comment 32

[Chris Peyer]

verified in argo ee91fa9c95a5