Are duplicated impl32 and implN fields really necessary?

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Virtual Machine
P5
normal
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Lars T Hansen, Assigned: Edwin Smith)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-bug +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Currently there are impl32/implN fields in both MethodInfo and MethodEnv; the latter are duplicates of the former to provide one-step access to the function pointer.  Rumor has it that this made a difference at some point.  However, at least on systems where memory is as important as speed the right thing to do would be to store the pointers only once.

We should investigate what might be done about this.
(Assignee)

Comment 1

9 years ago
at one time, coerceEnter invoked env->impl[32/N] directly.  Jit'd code still does, and env->impl is used in isInterpreted(), to decide to call the interpreter, but the coerceEnter variants use method->impl32 to invoke jit/native methods.  performance comparisons may be more valid if this is changed back to using env->impl32.

another factor is caching, if MethodInfos become transient then the impl pointer may need to stay in MethodEnv.
(Assignee)

Comment 2

9 years ago
its hard to measure any difference on intel cpu's but PPC builds with VMCFG_METHODENV_IMPL32=0 are slower than =1, suggesting the extra load in the call path of jit code calling jit code, is significant.

Comment 3

9 years ago
slower by how much? 1%, 10%, 100%...?
(Assignee)

Comment 4

9 years ago
Enough for me to decide it's worth keeping the copy in env, >10% slowdown in some tests.  I'll repost numbers next time I test the two configurations side-by-side again.

Note that the jit-code-cache feature requires the ability to fallback to the interpreter, which means updating all the trampolines at once, before freeing the jit'd code.  Naturally this is simple when there is only one pointer.

For now, we'll pay some jit-call performance for the ability fallback to the interpreter.  However we might be able to make it up some other way, maybe by flattening MethodEnv itself into a struct within VTable, ignoring the nasty fact that doing so would re-introduce more copies of the trampoline pointer.

Comment 5

9 years ago
if it's noticeable on PPC but not Intel, sounds like it's worth adding to the config system as an option -- no point in paying for the extra memory on platforms that it doesn't pay for itself.
(Assignee)

Comment 6

9 years ago
I intend to add AVMFEATURE switches for different code cache/execution policies, and these features will set/clear VMCFG_METHODENV_IMPL32 accordingly.
(Assignee)

Comment 7

9 years ago
It's a no brainer to disable the duplicate pointers in interpreter-only configs, any gain from one less load would be drowned by interpreter overhead.  

it's cleanest for this to be a VMCFG flag that is set only in certian feature combinations:

interpreter-only: not set
jit with no cache: set
jit with cache: not set

if the features are AVMFEATURE_JIT and AVMFEATURE_JIT_CACHE, and _JIT_CACHE requires JIT, how to make the flag be set properly?

its easy to make it a first class feature (with various REQUIRES or PRECLUDES) clauses; but it seems wrong; it's a tweak.

its also easy to set/clear the flag with custom logic in avmbuild.h, but that seems hairy.
(Assignee)

Comment 8

9 years ago
Created attachment 380114 [details] [diff] [review]
make VMCFG_METHODENV_IMPL32 be defined/not defined like other config flags, instead of always defined as 1/0
Assignee: nobody → edwsmith
(Reporter)

Updated

8 years ago
Priority: -- → P5
Target Milestone: --- → flash10.1
(Assignee)

Updated

8 years ago
Attachment #380114 - Attachment is obsolete: true

Updated

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

Updated

8 years ago
Assignee: edwsmith → nobody
(Assignee)

Comment 9

8 years ago
Created attachment 441502 [details] [diff] [review]
Set VMCFG_METHODENV_IMPL32 in jit configurations only

This sets VMCFG_METHODENV_IMPL32 in JIT configurations only, and uses the feature system instead of a hack in avmbuild.h
Assignee: nobody → edwsmith
Attachment #441502 - Flags: review?(stejohns)

Updated

8 years ago
Attachment #441502 - Flags: review?(stejohns) → review+
(Assignee)

Comment 10

8 years ago
TR: http://hg.mozilla.org/tamarin-redux/rev/5463c7ac2628
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All

Updated

7 years ago
Flags: flashplayer-bug+
You need to log in before you can comment on or make changes to this bug.