Several frequently-called AvmCore methods could be static, but aren't

VERIFIED FIXED

Status

Tamarin
Virtual Machine
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

unspecified
Bug Flags:
flashplayer-triage +

Details

Attachments

(1 attachment, 1 obsolete attachment)

111.35 KB, patch
Lars T Hansen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
e.g., number(), integer(), boolean(). Means one less argument to pass, and a tiny reduction in jitted code size.

Comment 1

10 years ago
these have to call valueOf() indirectly, they aren't static on purpose
(Assignee)

Comment 2

10 years ago
(In reply to comment #1)
> these have to call valueOf() indirectly, they aren't static on purpose

I don't understand... valueOf is a pure AS3 function, it doesn't exist in C++.

Comment 3

10 years ago
i stand corrected, i was thinking of double->Atom conversion too, but not in play here.  sure, if these become static and stuff compiles, super.  (CodegenLIR/MIR need updating, but otherwise no biggie).
(Assignee)

Comment 4

10 years ago
yep, and it moves the needle a little on some perf tests... up to 3% in a handful. Patch forthcoming later.
(Assignee)

Comment 5

10 years ago
Turns out that istype() can also be made static (and faster) by rewriting using the BUILTIN_ types, which allows a bunch of others to be staticified as well. Perf increase is modest but worthwhile.

Updated

10 years ago
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
(Assignee)

Comment 6

10 years ago
Created attachment 362623 [details] [diff] [review]
Patch

Initial patch that converts a slew of AvmCore methods to static. Performance increase is modest in JIT mode on most benchmarks, but math-centric benchmarks (eg primes, crypto) show 15-25% improvement in JIT mode. Also added some minor but worthwhile optimizations to the bodies of AvmCore::number, AvmCore::istype, Toplevel::add2

Further benchmarking and testing is necessary before landing (only MacTel benchmarking has been done), this is just an initial advisory patch... no rush on a review :-)
Attachment #362623 - Flags: review?(lhansen)

Comment 7

10 years ago
Comment on attachment 362623 [details] [diff] [review]
Patch

General approach seems fine.
Attachment #362623 - Flags: review?(lhansen) → review+
(Assignee)

Comment 8

9 years ago
Created attachment 366626 [details] [diff] [review]
Patch, rebased

same patch, rebased for current tip. this would be nice to get into redux before the next release to TC.
Assignee: nobody → stejohns
Attachment #362623 - Attachment is obsolete: true
Attachment #366626 - Flags: review?(lhansen)

Updated

9 years ago
Attachment #366626 - Flags: review?(lhansen) → review+
(Assignee)

Comment 9

9 years ago
pushed to redux as changeset:   1581:0af36f31b8fc
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 10

9 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED

Comment 11

9 years ago
removing QRB request, bug resolved/verified
Flags: flashplayer-qrb?
You need to log in before you can comment on or make changes to this bug.