Hello, apologies for the slow response, been busy as well. Thanks everyone for your input, it's been helpful understanding the general mindset of approaches and learning how things work.
Eric Miles: > If you want to try some smaller optimizations in the bytecode generation then > you’ll start to get some perspective on how easy/hard it can be to translate > AST into bytecode. > There is room for improvement in our bytecode, for sure. It just happens to > be the part of the code that is most full of pitfalls and weirdness and > requires a very steady hand and a lot of testing and sometimes trial and > error. I've been playing around with some ideas in the classgen and it indeed looks quite tricky... some of the compilation artefacts start making more sense now, I'll bear your word of caution in mind when working on it. Before actually looking at it, I was half-hoping there was some kind of special "Optimization phase" for providing more context for the classgen, although it seems like the optimizing statement writer is somewhat used for that purpose? I think I need to spend more time poking around the code to properly understand how it works. Jochen Theodorou: > InvokerHelper normally contains methods for the users of the language to > help the with mostly invocation related issues like invoking a method in > a dynamic way or such. There is more in there, but usually we only add > if really really needed. ScriptBytecodeAdapter (SBA) is for helper code > directly called from the bytecode and tailored to the needs of those > calls only. Good to know, in that case I'll go back to using SBA. > VMPlugin should not call into SBA or InvokerHelper. Should I be copying over code in that case? Or should it be calling DGM instead? > I actually now wonder why you do Arrays.stream(values).toList(); in the > Java16 version. Would List.of be better if all elements are not null? If > yes, then why not check them? And why go with List#of and catch-NPE > instead of stream before? Stream#toList ends up creating the same kind of object (with its JVM @Stable optimisations) as List#of (see java.util.stream.ReferencePipeline#toList). The difference is the former allows nulls, while the latter iterates over each parameter and throws an NPE if any are null. Using the try-catch approach is slower because in the worst-case scenario it would be performing lots of null checks only to give up and use Collections#unmodifiableList, which the JVM can't optimise as easily due to being backed by a mutable collection. > In the end though I > second Miles and wonder if it is worth it. Java 9's immutable collections can have meaningful performance advantages for grabbing data (akin to records) and avoiding redundant copying, although in retrospect, I agree that my current approach is a bit over the top in the context of the generated toList()/toMap() record methods. Paul King: > what do you think of this simplified PR: > https://github.com/apache/groovy/pull/2047 > > It could be made to know about emptyList(), singletonList() etc., but I am > not sure the added complexity adds much value. Admittedly, I got carried away with optimising for every scenario. Technically my approach maximises the chances of using the special immutable collections and minimising GC pressure (all empty lists reference the same field for example) - both at compile-time and runtime - however it's a lot more code than just simply wrapping it in an unmodifiable list. I'm happy with your simplified PR if you'd like to go ahead with that. Later on, I could re-evaluate my approach to be less micro-optimised and/or more generalised so that it could benefit other parts of the Groovy language to better justify its complexity. Best regards, Oscar ________________________________ From: Jochen Theodorou <blackd...@gmx.org> Sent: 27 January 2024 11:38 To: dev@groovy.apache.org <dev@groovy.apache.org> Subject: Re: Questions regarding implementation approach for GROOVY-11294 Hi Oscar, sorry for replying so late, I was very busy this week. On 22.01.24 20:55, Oscar N wrote: > Indeed, that is the crux of my earlier questions. I've got it working at > runtime, however I'm unsure of which approach to take for compile-time. > > Adding new `ScriptBytecodeAdapter`/`InvokerHelper` methods that call a > `VMPlugin` method allows for optimising other scenarios, such as > `DefaultGroovyMethods.asImmutable(List)`. InvokerHelper normally contains methods for the users of the language to help the with mostly invocation related issues like invoking a method in a dynamic way or such. There is more in there, but usually we only add if really really needed. ScriptBytecodeAdapter (SBA) is for helper code directly called from the bytecode and tailored to the needs of those calls only. Used mostly if too complex to compile directly to bytecode or if further helper structures like the VMPlugin are required. Here again the rule is trying to avoid using it, but the VMPlugin is not for direct calling, so if you do like you did in your started implementation, then going through SBA looks to me like the right way. DefaultGroovyMethods are methods made available by the runtime for method calls. DefaultGroovyMethods.asImmutable(List) would mean List#asImmutable() ah yes.. and VMPlugin should not call into SBA or InvokerHelper. > You can find my current implementation here: PaintNinja/groovy at > GROOVY-11294-record-immutable-tolist (github.com) > <https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPaintNinja%2Fgroovy%2Ftree%2FGROOVY-11294-record-immutable-tolist&data=05%7C02%7C%7C9674845022244ec5045e08dc1f2c8ad6%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638419523376485393%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=rcYmeVpSJw1X5UfUGmSGFfxBPfM8yoRQsZp62jk3nqQ%3D&reserved=0<https://github.com/PaintNinja/groovy/tree/GROOVY-11294-record-immutable-tolist>> well.. I suggested some changes above already. In the end though I second Miles and wonder if it is worth it. > As for compile-time optimisations, I'm not very familiar with Groovy's > compiler internals. What would be the most suitable place to do JVM > version-specific optimisations? For bytecode generation we ask the supported version in the compiler config. For the runtime VMPlugin is the right place. > Note that the exact type of immutable list used also varies on the > nullity of items and the length of the array, so for optimal performance > it's unfortunately not as simple as one method for all lists on one Java > version and another method on a different version. I actually now wonder why you do Arrays.stream(values).toList(); in the Java16 version. Would List.of be better if all elements are not null? If yes, then why not check them? And why go with List#of and catch-NPE instead of stream before? bye Jochen
