ArrayObject::getUintProperty needs explicit inlining

RESOLVED FIXED

Status

Tamarin
Verifier
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Steven Johnson, Assigned: Steven Johnson)

Tracking

Details

Attachments

(1 attachment)

12.89 KB, patch
Lars T Hansen
: review+
Edwin Smith
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Performance on some benchmarks (eg Euler.as) is highly sensitive to ArrayObject::getUintProperty being fast. In the current compiler configuration, it happens that _getUintProperty is inlined into the (virtual) implementation of getUintProperty; however, it seems that minor tweaks to the contents (eg methods of AtomArray) can change this behavior, causing getUintProperty to tail-call to _getUintProperty instead, which is significantly slower. We should restructure the code to explicitly force the function to be inlined, to avoid such inadvertent slowdowns.
(Assignee)

Comment 1

8 years ago
Created attachment 465003 [details] [diff] [review]
Patch

-- Move contents of _getUintProperty into a really-inlined getUintPropertyImpl.
-- (virtual) getUintProperty, _getUintProperty, and _getIntProperty all explicitly call getUintPropertyImpl.
-- added note to the _get/_set int/uint property calls that they are only for use by JIT or ArrayObject. (friend access is painful due to the way they are used by jit-calls.h, so a comment will have to suffice.)
-- scope creep: get/setIntProperty (no underscore) aren't called anywhere in the VM, and only in a handful of places in Flash/AIR, and all of those are inappropriate (getUintProperty is more appropriate as the index is always positive)... so they were removed entirely.
-- scope creep: added some missing "-inlines.h" files to the xcode project.
Assignee: nobody → stejohns
Attachment #465003 - Flags: superreview?(edwsmith)
Attachment #465003 - Flags: review?(lhansen)

Comment 2

8 years ago
Comment on attachment 465003 [details] [diff] [review]
Patch

Bonus points if you explain in a comment /how/ _getUintProperty differs from getUintProperty, and not simply that most users should call the latter.  (With the proviso that I'm just looking at the patch and not seeing enough context.)
Attachment #465003 - Flags: review?(lhansen) → review+
(Assignee)

Comment 3

8 years ago
Point taken, patch will be annotated with comments from this bug before pushing.
(Assignee)

Comment 4

8 years ago
pushed to redux as http://hg.mozilla.org/tamarin-redux/rev/7b69fbca6b8f
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Attachment #465003 - Flags: superreview?(edwsmith) → superreview+
You need to log in before you can comment on or make changes to this bug.