Devirtualization of method calls

RESOLVED WONTFIX

Status

P3
enhancement
RESOLVED WONTFIX
9 years ago
6 days ago

People

(Reporter: kpalacz, Assigned: kpalacz)

Tracking

unspecified
Q3 12 - Dolores
Dependency tree / graph
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: PACMAN, has-patch)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
Created attachment 437678 [details] [diff] [review]
Devirtualize calls to final, jit-compiled methods.

Early experiment, no promises.

Comment 2

9 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

9 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.

Updated

9 years ago
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
(Assignee)

Comment 4

9 years ago
Created attachment 438213 [details] [diff] [review]
handling of native, call target patching.

Messy proof of concept, x86-only patching, one acceptance test fails.
Attachment #437678 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Created attachment 438607 [details] [diff] [review]
page protection shenanigans 

workaround, all acceptance tests pass (obviously not an acceptable solution though).
Attachment #438213 - Attachment is obsolete: true

Comment 6

9 years ago
Any preliminary performance #s to report?
(Assignee)

Comment 7

9 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

9 years ago
Created attachment 441642 [details] [diff] [review]
better handling of page protection.

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

9 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.
Depends on: 563146, 511873
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
The patch already tries to do that, but it makes sense to factor out.

Updated

9 years ago
Component: Virtual Machine → JIT Compiler (NanoJIT)

Updated

8 years ago
Flags: flashplayer-bug-
Whiteboard: has-patch

Updated

8 years ago
Depends on: 641055

Comment 11

8 years ago
Is this still valid?
Assignee: nobody → kpalacz
No longer depends on: 641055
Flags: flashplayer-injection-
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan

Updated

8 years ago
Depends on: 641055
(Assignee)

Comment 12

8 years ago
Not actively worked on at the moment, but valid.

Comment 13

7 years ago
Krzys, shall this bug be deferred to Cyril?

Updated

7 years ago
Whiteboard: has-patch → PACMAN, has-patch
(Assignee)

Comment 14

7 years ago
yes, it's a performance enhancement, not actively worked on.

Updated

7 years ago
Target Milestone: Q1 12 - Brannan → Q3 12 - Dolores

Comment 15

7 years ago
Dropping from Andre watch list, as it has target milestone.

Updated

7 years ago
No longer depends on: 641055

Updated

7 years ago
Depends on: 733017
Status: NEW → RESOLVED
Last Resolved: 6 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.