Exception handling may be slow and use a lot of space

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
normal
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Lars T Hansen, Assigned: Krzysztof Palacz)

Tracking

(Blocks: 1 bug)

unspecified
flash10.1
x86
Mac OS X
Bug Flags:
flashplayer-qrb +

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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.
(Reporter)

Updated

9 years ago
Summary: Exception handling may be slow → Exception handling may be slow and use a lot of space
(Reporter)

Updated

8 years ago
Priority: -- → P4
Target Milestone: --- → flash10.1

Updated

8 years ago
Target Milestone: flash10.1 → flash10.2
(Reporter)

Comment 2

8 years ago
There was a recent thread on LEGO about this, with some numbers.  Adobe devs: contact me about it if you wish.
(Assignee)

Comment 3

8 years ago
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.
(Reporter)

Comment 4

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

Comment 5

8 years ago
Created attachment 425837 [details] [diff] [review]
Preliminary patch, intended for testing.
(Reporter)

Comment 6

8 years ago
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.
Assignee: nobody → kpalacz
Priority: P4 → P2
Target Milestone: flash10.2 → flash10.1
(Assignee)

Comment 7

8 years ago
Created attachment 426008 [details] [diff] [review]
adds comments

Started working on asserts but not done.
Attachment #425837 - Attachment is obsolete: true
Attachment #426008 - Flags: review?(lhansen)
(Reporter)

Comment 8

8 years ago
Comment on attachment 426008 [details] [diff] [review]
adds comments

Good so far.  Probably "routinve" should be "routine".
Attachment #426008 - Flags: review?(lhansen) → review+
(Assignee)

Comment 9

8 years ago
Created attachment 426018 [details] [diff] [review]
Fixes typo
Attachment #426008 - Attachment is obsolete: true
(Assignee)

Comment 10

8 years ago
Created attachment 426031 [details] [diff] [review]
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.
Attachment #426031 - Flags: review?(lhansen)
(Reporter)

Updated

8 years ago
Attachment #426031 - Attachment is patch: true
Attachment #426031 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 11

8 years ago
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).
Attachment #426031 - Flags: review?(lhansen) → review-
(Assignee)

Comment 12

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

Comment 13

8 years ago
Created attachment 426061 [details] [diff] [review]
typos and cosmetics
Attachment #426045 - Attachment is obsolete: true
Attachment #426045 - Flags: review?(lhansen)
(Assignee)

Updated

8 years ago
Attachment #426061 - Flags: review?(lhansen)
(Reporter)

Comment 14

8 years ago
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.
Attachment #426061 - Flags: review?(lhansen) → review+
(Assignee)

Comment 15

8 years ago
Created attachment 426083 [details] [diff] [review]
Incorporated latest comments
Attachment #426061 - Attachment is obsolete: true

Updated

8 years ago
Blocks: 545409

Comment 16

8 years ago
Narrowing this bug's focus to setjmp signal handling change.  Stack space investigation separated to Bug 545409 and marked for Future.
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
(Assignee)

Comment 17

8 years ago
Created attachment 426286 [details] [diff] [review]
Moves signal utilities to PosixPortUtils.cpp

MacDebugUtils.cpp included in all builds.
Attachment #426083 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Updated

8 years ago
Attachment #426018 - Attachment is patch: true
Attachment #426018 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 19

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

Consider this patch approved.

Comment 20

8 years ago
(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

8 years ago
(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

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

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

Comment 23

8 years ago
(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

8 years ago
Yes, let's investigate  at the situation on Linux too.
(Assignee)

Comment 25

8 years ago
Created attachment 427190 [details] [diff] [review]
Equivalent fix for Linux.
Attachment #427190 - Flags: review?(lhansen)
(Reporter)

Comment 26

8 years ago
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.
Attachment #427190 - Flags: review?(lhansen) → review-
(Assignee)

Comment 27

8 years ago
Created attachment 427221 [details] [diff] [review]
Incorporated review comments, added JIT fix (affects the Mac as well)

Tentative, seeing significant improvement on the Mac on at least one benchmark.
Attachment #427190 - Attachment is obsolete: true
Attachment #427221 - Flags: review?(lhansen)

Comment 28

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

shouldn't __linux__ be AVMPLUS_LINUX?
(Reporter)

Comment 29

8 years ago
(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.

:-)
(Reporter)

Comment 30

8 years ago
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.
Attachment #427221 - Flags: review?(lhansen) → review+
(Assignee)

Updated

8 years ago
Attachment #427221 - Flags: superreview?(edwsmith)

Updated

8 years ago
Attachment #427221 - Flags: superreview?(edwsmith) → superreview+

Comment 31

8 years ago
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

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 32

8 years ago
verified in argo ee91fa9c95a5
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.