uschindler commented on PR #15285: URL: https://github.com/apache/lucene/pull/15285#issuecomment-3518793552
Hi, Sorry for the delay. The FFM part was already moved out of the separate sourceSet in main branch (we no longer have a mrjar in reality, it's copied together, we just have separate compilation units to allow is to compile against a stable API and optionally add MRJAR when panama changes in later have versions). The separation was already removed... except the glue classes for vectors. So MmapDir and madvise is already part of main sources. So the issue description is wrong. This is only about paname vectors. What's done here is just moving some glue classes to allow panama vector to access memory segments provided by MmapDir and possibly a new MemorySegment backed replacement of Bytebuffersdirectory to the main sources. This is needed and ok to do, but my main issue with this PR is the additional complexity just to achieve this! Why do we need all those additional abstractions with functional interfaces everywhere? I tried to understand this and every time I looked at this PR I gave up after 20 minutes starring at those horrible abstractions with generics. Let's have exactly one interface preferably without generics as glue part between panama vector in separate sourceSet. Please make it simpler or give a full explanation why we need all those extra generics and functional interfaces. The pr adds 400 extra lines of code instead of making it simpler! Another problem which makes reviewing harder is the additional renames of methods. Can we separate that to make it easier to get a glimpse what's going on? Sorry for the delay but it's busy here and that's too.much complexity for a quick review. Maybe @rmuir can also have a look. And finally: we have some benchmarks here, but those are too simple to show how the additional abstractions affect hotspot compiler of executed in real code. A micro benchmark like this has lead to problems suddenly only appearing on Mike's benchmarks. Because if you benchmark a little bit of code with abstractions, hotspot has an easy job to remove the abstractions. But in complex environments during query execution the additional abstractions can kill your performance! So because of this I am really afraid of this PR to go in in that current form. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
