-
Notifications
You must be signed in to change notification settings - Fork 240
Qualified methods #993
base: master
Are you sure you want to change the base?
Qualified methods #993
Conversation
…eDefinition/MethodDefinition may be used.
As @kg supposed, this PR really increase memory usage. I've not supposed that memory usage increase will be so big. Will be glad for any help/suggestions what we can do with it |
@@ -3014,6 +3041,26 @@ JSIL.InstantiateProperties = function (publicInterface, typeObject) { | |||
$jsilcore.CanFixUpEnumInterfaces = false; | |||
|
|||
JSIL.FixupInterfaces = function (publicInterface, typeObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main changes is inside this method. It now calculates proper virtual member implementation for each base type, not only implemented interfaces.
Check is stricter now - method should be virtual and for auto-binding by name to interface class must explicitly declare that interface in it's own declaration.
@kg, why do we cache MethodSignatures/TypeRefernce per type instead of caching them per assembly? |
@iskiselev Mostly because it was easier. I see no reason not to do it assembly-wide. |
3c3325d
to
4efdd00
Compare
I've broke #624 with null arg to instance method with this change. I believe it's OK, as nobody should really write such code. We still may fix it later. |
Looks like I'm nearly done with it. The only left is moving Call method creation from MethodSignature to InterfaceMethod and small cleanup of code inside MethodSignature / InterfaceMethod.
|
@kg, I think we could not use MethodSignature/InterfaceMethod Call/CallNonVirtual/CallStatic at all.
We will use for non-variance call:
And for variance calls:
I believe it should not have any performance issues, but will improve performance/memory footprint as we will not create Call method closures. What do you think? |
It looks gross, but it might be considerably better for performance, so maybe it's worth it. It does solve the this-reference binding problem and it eliminates most of the need for closures, which is nice. |
170c5d6
to
13411dd
Compare
With JavaScript you can get performance problem nearly anywhere. Chrome | Accessor: 580 ms first calls, 130 ms next calls (not always, sometimes it stays 580 forever) Firefox | Accessor: 210 ms Edge | Accessor: 800-900 ms @kg, have you any ideas what we could do with it? Also I'm interesting how arguments count will affect this numbers. Oh, Edge works in real-life better than in Test. Strange.
Update: test result updated based on chakra-core/ChakraCore#1153 (comment) |
Looks like the only way to achieve normal performance would be calculate function overload name using some stable algorithm and hardcode them on translation. Otherwise we will be much slower comparing to .Net. |
Results for stable: |
We definitely need report bug to Chackra, as both Signature and Stable takes too long, Even Prototype takes longer, then others. I just not sure how it should be described. |
The solution here is probably (ugh) to move to shipping some sort of bytecode in Release builds, and then translate the bytecode to JS at loadtime. That translated JS can have all the keys hardcoded as |
@kg, tried create small sample with run-time generated functions and added it to sample: As I can see, Chakra works bad with run-time generated functions (will create simplified sample and report, if it is not reported yet). It's interesting for me, why Firefox works 3 times slower comparing to Signatures case. But still it works fast enough to try it. |
My long-term strategy for bytecode would be to move things around such that the existing ASTEmitter can run in the browser, and then we ship a serialized version of the JS AST to it over the wire. For debug builds we would just run the ASTEmitter up front and save the output .js files. |
It really sounds great, but I need to solve problem right now. Looks like hacking AST would be the fastest way to do it. |
Safari also likes Stable/Generated approaches. They run faster than any else. |
If this is blocking you right now, it sounds like a short-term solution is I'm okay with a stable compile-time name generation approach as long as we On 17 June 2016 at 15:33, Igor Kiselev [email protected] wrote:
|
Right now I'm planning use "Generated" approach. So, each function will be written as sting with includes of run-time calculated signatures. Something like:
I know, it really looks ugly, but I believe that it could be done in just few days and may give big performance improvement. |
Yikes. Well, as long as it's an option... I'd like to find a better On 17 June 2016 at 18:17, Igor Kiselev [email protected] wrote:
|
Really I'll use it only for release build, but if it gives me 2-5 times performance improvement I'll accept it. If performance improvement will be less than 2 times, I'll not use it. |
Yeah if it's that much faster it's worth it. Ideally we can just improve As-is all that concatenation means you can't effectively ctrl-f in the On 17 June 2016 at 18:21, Igor Kiselev [email protected] wrote:
|
Templates would not help us with ctrl-f, as we plan to modify string. |
If we will talk about "Stable" names approach, the main problem now is that I can't found way to encode function for interfaces. If we'll find such way, it would be also pretty good option. |
This PR is not yet finished, but I'd like to start discussions for it. Main goal here is implement #991 and fix #185, #623.
Instead of MethodSignature for method calls used InterfaceMethod (that is just signature+name+source type).
Also fixed huge number of small bugs that was revealed during implementation (mostly with very edge cases, so they was not noticed before).