Closed
Bug 557926
Opened 15 years ago
Closed 6 years ago
Devirtualization of method calls
Categories
(Tamarin Graveyard :: Baseline JIT (CodegenLIR), enhancement, P3)
Tamarin Graveyard
Baseline JIT (CodegenLIR)
Tracking
(Not tracked)
RESOLVED
WONTFIX
Q3 12 - Dolores
People
(Reporter: kpalacz, Assigned: kpalacz)
References
Details
(Whiteboard: PACMAN, has-patch)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
In many cases targets of method invocations are either constant or change infrequently. Avoiding indirect calls may improve performance (even if the caller has to pass MethodEnv).
Assignee | ||
Comment 1•15 years ago
|
||
Early experiment, no promises.
Comment 2•15 years ago
|
||
nice.
one problem with the patch: initially, even jit methods are not verified, and get verified on first call. So in your patch, I think the patch has a chance of directly calling the verify trampoline. (and re-calling it over and over). why verify native methods on first call? well, there's no good reason now, but we just to jit-compile thunks. Now, the if(isNative()) branch in MethodInfo::verify() installs the thunk pointers, and there are no failure modes, IIRC.
Possible fix: The if(isNative()) branch in MethodInfo::verify() can be hoisted to resolve time. (I did this in the now-defunct jit-cache experiment last year). Every method is guaranteed to be resolved before being a) early bound to at a call site, or b) verified. By setting up the thunk addresses at resolve time, an early bound call site to a final native method will see the final value of MethodEnv.implGPR at JIT time.
in principle its also possible to inline the thunk, but thats another experiment.
there's another very minor gotcha: when debugger is enabled, the address called will be debugEnterExitWrapper32, which then calls the underlying native method whose address is stored in a different field. directly calling this wrapper probably doesn't help much, but also doesn't hurt much either.
Comment 3•15 years ago
|
||
never comment before coffee: i read "jit compiled" as "native". the patch should work fine for jit-compiled methods, although we'll need to patch that call address when deopt comes along.
and of course it will work for native calls to if we hoist the if(isNative()) branch in MI::verify() like i mentioned.
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Assignee | ||
Comment 4•15 years ago
|
||
Messy proof of concept, x86-only patching, one acceptance test fails.
Attachment #437678 -
Attachment is obsolete: true
Assignee | ||
Comment 5•15 years ago
|
||
workaround, all acceptance tests pass (obviously not an acceptable solution though).
Attachment #438213 -
Attachment is obsolete: true
Comment 6•14 years ago
|
||
Any preliminary performance #s to report?
Assignee | ||
Comment 7•14 years ago
|
||
Preliminary results are flat, I suspect that too few call sites can be devirtualized on larger benchmarks, and microbenchmarks are so small, that hardware support for indirect calls is adequate (e.g. branch target buffer misses are rare).
More call sites could be devirtualized if we either had OSR or did preexistence analysis (I was considering doing the latter). Also, the calling sequence is so long (bug 511873) that perhaps the effects of devirutalization won't be seen until it's pared down.
Assignee | ||
Comment 8•14 years ago
|
||
Saves code allocation info with the patch address so that the appropriate chunk can be unprotected for the duration of patching.
Attachment #438607 -
Attachment is obsolete: true
Comment 9•14 years ago
|
||
bug 563146 will set up native method invocation ponters sooner, so that when early binding to a final native method, we can call it directly with no patching and no indirection.
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9)
The patch already tries to do that, but it makes sense to factor out.
Updated•14 years ago
|
Component: Virtual Machine → JIT Compiler (NanoJIT)
Updated•14 years ago
|
Flags: flashplayer-bug-
Whiteboard: has-patch
Comment 11•14 years ago
|
||
Is this still valid?
Assignee: nobody → kpalacz
No longer depends on: Andre
Flags: flashplayer-injection-
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan
Assignee | ||
Comment 12•14 years ago
|
||
Not actively worked on at the moment, but valid.
Comment 13•13 years ago
|
||
Krzys, shall this bug be deferred to Cyril?
Updated•13 years ago
|
Whiteboard: has-patch → PACMAN, has-patch
Assignee | ||
Comment 14•13 years ago
|
||
yes, it's a performance enhancement, not actively worked on.
Comment 15•13 years ago
|
||
Dropping from Andre watch list, as it has target milestone.
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•