Closed charliegracie closed 7 years ago
Looks good @charliegracie :smile:
One question before I merge though: have you done any performance testing for your last two commits (ilgen for OP_CALL and OP_RETURN)? I've been thinking a lot about the implications of these changes. I'm concerned that it may only improve performance on some micro-benchmarks, but actually reduce performance in the general case. The additional ilgen will cause many more nodes to be generated that will likely confuse some JIT optimizations, causing the generated code to run slower.
I'm willing to merge those two commits if you think it will significantly improve performance on benchmarks we care about. Otherwise, I would prefer they be excluded from this PR.
I don't want to lose this work though as I think it could still be very valuable. If we implemented the right inlining heuristics, we could ilgen luaD_precall
and luaD_poscall
as helpers and let the inliner figure out whether their worth inlining or not. This would greatly improve our cache locality.
To this end, you could create a new PR with those two commits as some initial work.
I will get back to this when I am back from FOSDEM or if I have time while I am there.
I have removed the OP_CALL and OP_RETURN implementations for now. I have them in a separate branch and we can revisit that in the future