476769 - Are duplicated impl32 and implN fields really necessary?

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

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.

Comment 1

[Edwin Smith]

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.

Comment 2

[Edwin Smith]

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

[Steven Johnson]

slower by how much? 1%, 10%, 100%...?

Comment 4

[Edwin Smith]

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

[Steven Johnson]

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.

Comment 6

[Edwin Smith]

I intend to add AVMFEATURE switches for different code cache/execution policies, and these features will set/clear VMCFG_METHODENV_IMPL32 accordingly.

Comment 7

[Edwin Smith]

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.

Comment 8

[Edwin Smith]

Attachment: make VMCFG_METHODENV_IMPL32 be defined/not defined like other config flags, instead of always defined as 1/0

Comment 9

[Edwin Smith]

Attachment: 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

Comment 10

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/5463c7ac2628