sq / JSIL

CIL to Javascript Compiler
http://jsil.org/
Other
1.73k stars 241 forks source link

Qualified methods #993

Open iskiselev opened 8 years ago

iskiselev commented 8 years ago

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

iskiselev commented 8 years ago

As @kg supposed, this PR really increase memory usage. I've compared my app compiled with CacheOneMethodSignaturePerMethod before this PR and after it. Here is memory usage according to Chrome Take Heap Snapshot: Before: 358Mb After: 508Mb

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

iskiselev commented 8 years ago

@kg, why do we cache MethodSignatures/TypeRefernce per type instead of caching them per assembly?

kg commented 8 years ago

@iskiselev Mostly because it was easier. I see no reason not to do it assembly-wide.

iskiselev commented 8 years ago

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.

iskiselev commented 8 years ago

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. What should be one later (not inside this PR, but when I'll have more time to work on it):

iskiselev commented 8 years ago

@kg, I think we could not use MethodSignature/InterfaceMethod Call/CallNonVirtual/CallStatic at all. Instead we'll use methodKey from InterfaceMethod. So, instead of:

QS01().Call(thisReference, [gArg1, gArg2], arg1)
QS01().CallNonVirtual(thisReference, [gArg1, gArg2], arg1)
QS02().CallStatic([gArg1, gArg2], arg1)

We will use for non-variance call:

thisReference[QS01().Key](gArg1, gArg2, arg1);
thisReference[QS01().NonVirtualKey](gArg1, gArg2, arg1);
(T01()[QS02.Key])(gArg1, gArg2, arg1) // here we may return back using just MethodSignature

And for variance calls:

thisReference[QS01().LookupVariantMethodKey(thisReference)](gArg1, gArg2, arg1);

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?

kg commented 8 years ago

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.

iskiselev commented 8 years ago

With JavaScript you can get performance problem nearly anywhere. I've tested my idea with using MethodKey. Here is example: http://plnkr.co/edit/hVsmbE?p=preview I've rewrite in plain JavaScript our a little bit simplified BaseMethodCall performance test. There Accessor is something similar to my last idea, Signature - as it worked with Call using InterfaceMethod function, and Prototype - how non-virtual call works before. So, here is result: .Net: 30-50ms

Chrome | Accessor: 580 ms first calls, 130 ms next calls (not always, sometimes it stays 580 forever) Chrome | Signature: 75 ms Chrome | Prototype : 20 ms Chrome | Stable: 22 ms

Firefox | Accessor: 210 ms Firefox | Signature: 11 ms Firefox | Prototype : 11 ms Firefox | Stable: 11 ms

Edge | Accessor: 800-900 ms Edge | Signature: 970 ms Edge | Prototype: 70 ms first calls, 25 ms next calls Edge | Stable: 650-1050 ms

@kg, have you any ideas what we could do with it?

Also I'm interesting how arguments count will affect this numbers. In real JSIL-translated apps numbers are even worth: Chrome | Signature: 275 ms Firefox | Signature: 150 ms Edge | Signature: 280-350 ms

Oh, Edge works in real-life better than in Test. Strange.

Type .Net Chrome Demo Firefox Demo Edge Demo Chrome JSIL Firefox JSIL Edge JSIL
Accessor 30-50 580, 130 210 800-900 850-1050 280 900-950
Signature 30-50 75 11 140, 100 70, 30 150 280-350
Prototype 30-50 20 11 70, 25 180-210 160 260
Stable 30-50 22 11 55, 20
Generated 30-50 22 33 (11 on Nightly) *90

Update: test result updated based on https://github.com/Microsoft/ChakraCore/issues/1153#issuecomment-228178952

iskiselev commented 8 years ago

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.

iskiselev commented 8 years ago

Results for stable: Chrome | Stable: 22 ms Firefox | Stable: 11 ms Edge | Stable: 650-1050 ms

iskiselev commented 8 years ago

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.

kg commented 8 years ago

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 x["y"] or x.y. I already wanted to do this and have some plans (there's a design Issue open about it) but I don't know when I'll have the energy to actually work on it.

iskiselev commented 8 years ago

@kg, tried create small sample with run-time generated functions and added it to sample: Chrome | Generated: 22 ms Firefox | Generated: 33 ms (11 on Nightly) Edge | Generated: 850-1050 ms

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. I think I'll be able to hack our AST Emitter, so it will create stings with run-time calculated inserts instead of function signatures. It's also interesting what should we do with type cache. Probably it is used not so often, or we may create global dictionary that will hold resolved type by string type it.

kg commented 8 years ago

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.

iskiselev commented 8 years ago

It really sounds great, but I need to solve problem right now. Looks like hacking AST would be the fastest way to do it.

iskiselev commented 8 years ago

Safari also likes Stable/Generated approaches. They run faster than any else.

kg commented 8 years ago

If this is blocking you right now, it sounds like a short-term solution is in order. I'm not really sure what makes the most sense here.

I'm okay with a stable compile-time name generation approach as long as we can make it comprehensive. The reason I've been relying on doing it at runtime is that there are a ton of edge cases to consider, and ILSpy/Cecil (especially ILSpy) don't give us 100% trustworthy information to use for generating names.

On 17 June 2016 at 15:33, Igor Kiselev notifications@github.com wrote:

Safari also likes Stable/Generated approaches. They run faster than any else.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sq/JSIL/pull/993#issuecomment-226897148, or mute the thread https://github.com/notifications/unsubscribe/AAMF8s_2kJVMS10DcwjQ_nvlV6-pAIirks5qMyDSgaJpZM4IxiPn .

iskiselev commented 8 years ago

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:

    $.Method({Static:false, Public:true }, "Method", 
      JSIL.MethodSignature.Void, 
      new FunctionCreator(function() {return ""+
        "this[" + $asm00.Base.$Methods.Method.InterfaceMethod.Of(JSIL.MethodSignature.Void).methodKey + "]();" + "\r\n" +
        "JSIL.Types[" + $asm00.Program.__TypeId__ +"].I = (((JSIL.Types[" + $asm00.Program.__TypeId__ +"].I | 0) + 2) | 0);"})
    );

I know, it really looks ugly, but I believe that it could be done in just few days and may give big performance improvement.

kg commented 8 years ago

Yikes. Well, as long as it's an option... I'd like to find a better solution than that.

On 17 June 2016 at 18:17, Igor Kiselev notifications@github.com 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:

$.Method({Static:false, Public:true }, "Method",
  JSIL.MethodSignature.Void,
  new FunctionCreator(function() {return ""+
    "this[" + $asm00.Base.$Methods.Method.InterfaceMethod.Of(JSIL.MethodSignature.Void).methodKey + "]();" + "\r\n" +
    "JSIL.Types[" + $asm00.Program.__TypeId__ +"].I = (((JSIL.Types[" + $asm00.Program.__TypeId__ +"].I | 0) + 2) | 0);"})
);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sq/JSIL/pull/993#issuecomment-226913478, or mute the thread https://github.com/notifications/unsubscribe/AAMF8p7lmdFKITiWJavXHkrq2BkPmTR6ks5qM0czgaJpZM4IxiPn .

iskiselev commented 8 years ago

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.

kg commented 8 years ago

Yeah if it's that much faster it's worth it. Ideally we can just improve the quality/readability of that code a little. Like instead of big string concats, a simple template/regex replacement operation done by FunctionCreator. Then the function bodies can at least be single string literals (or lists of strings, one per line).

As-is all that concatenation means you can't effectively ctrl-f in the output sources and it's much harder to breakpoint them. I'm not sure how it would interact with source maps, either.

On 17 June 2016 at 18:21, Igor Kiselev notifications@github.com 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sq/JSIL/pull/993#issuecomment-226913688, or mute the thread https://github.com/notifications/unsubscribe/AAMF8sDs6bOIHak9Q9NBcr4UX_2NG7Lkks5qM0gmgaJpZM4IxiPn .

iskiselev commented 8 years ago

Templates would not help us with ctrl-f, as we plan to modify string. I've thought about sourcemaps. We really can create sourcemap back to js file from generated function easy enough, if we additionally encode info about line where function was originally defined. It is also possible to make sourcemap back to .net sources, but it will take some more work.

iskiselev commented 8 years ago

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.