Port Views performance analysis functionality in D7 version to D8

JmOkay:

I'm curious for your thoughts on an alpha release of the D8 branch.

On the one hand, it'd be good to create the Views performance analysis functionality so that the features are fully congruent between D7 and D8 first.

But on the other hand, since everything else works in the D8 branch and all that's missing is performance analysis, it might be good if I made an alpha1 release ASAP so that we can increase adoption of the D8 version and get more people looking at it and submitting any issues before a beta would hit.

Do you have a preference whether alpha1 would get released now or wait until this issue is closed first? Thanks for your feedback!

Thanks for your feedback, it was very useful. tl;dr I'll go ahead and release an alpha version ASAP!

To continue the discussion about the best way to code the analysis:

My main concern with not keeping the monolithic class inheritance structure was in separating the logic. I wanted to make sure we had clear interfaces designed to do one thing, for representing each discrete part of the system. In the case of an entity pager, to me it seems like this boils down to there being the concept of an EntityPager, an EntityPagerLink, and an EntityPagerAnalyzer.

The analyzer isn't designed just to analyze a specific class, but the whole system while it's active (The pager, the links, the view, the current entity, whatever makes sense). Also, the analyzer isn't really supposed to be a mediator; the Analyzer is directly what's responsible for the entirety of the analysis. My implementation of the analyzer fires off events to further decouple the analysis, but since it's a separate, discrete class it'd be easy to do something else instead as the system does not rely on the analysis being event-based.

The contract of the EntityPagerAnalyzerInterface and implementation of the EntityPagerAnalyzer class ensures everything happens consistently and at exactly the right time. The fact that the two default events are included in the core module means they can be relied upon to run every time as well.

I'm definitely open to discussing other ways to accomplish this decoupling that wouldn't require the event system. Perhaps using a Plugin system instead of firing off an Event would be preferable, though that feels more heavy to me when the analysis doesn't have to involve much code as it is. What I'd like to avoid if possible is making the EntityPager class responsible for doing too much. It's already managing the view, handling its links, and storing its options, so it seems like the analysis portion is a good candidate for being able to live decoupled form that core class somehow.

One thing that could possibly make what's happening clearer, and do more to prevent misuse, could maybe be to extend the EntityPagerAnalyzerInterface a bit so that instead of just an analyze() function, it includes specific methods for defining how it happens, such as a method for retrieving the analyzed logs (so that the analyze() class isn't responsible for doing the logging).

Anyway, just wanted to lay out my thinking with this functionality and open it up to further discussion so we can hopefully find a good middle path forward before I start coding out the analysis. Thanks again!

First Alpha released, and I linked to this issue in the release notes as a remaining item before beta. Thanks again for your feedback on this!

Thanks again for your thoughts!

Regarding my reference to it being a monolithic class, I was referring mainly to the inheritance structure--even though each class only adds several methods, by the time you get to the outermost class that actually gets interacted with and inherits from all the others, it's got a lot of methods and is responsible for doing a lot of things. I don't think that's necessarily a bad thing or the wrong way to do it, but it became harder for me to think about extending the class structure in that way, which prompted me to split out the classes so they each are responsible for one primary thing and each have their own contract.

I see your point about the links; a separate class isn't strictly required there, and maybe it should be simplified. I wanted to encapsulate the functionality of determining how to generate it based on the view result row provided. If there's no associated result row, it works differently, but no other classes need concern themselves with that logic, they just ask for link markup. I do like thinking about that logic separately, but I wonder if there's a better (and more lightweight) way to handle it than a class instantiated for each link. I've got a couple ideas there, I may do some performance testing to see if I can find a lighter weight solution that still encapsulates the link generation as much as possible (perhaps a single static class responsible for generating pager links?).

I'm still putting some thought into the best way to handle the analysis in the current D8 structure given my original intent and your responses. I'll take a crack at getting that together soon, and whichever route I go we can evaluate and always decide to revisit again.

Btw, I know you've had a strong focus on performance in the Entity Pager module, so I plan to continue that a focus in my work on the D8 version. My initial goal is just to get all the functionality together in a logical way, and then I think when we hit Beta I'd like to start some performance tuning and refactoring in preparation for the first RC. By the time 1.0 hits, I'd like to try and make sure it's on par with the performance numbers you listed on the main module page (Are those accurate and recent? Was there a certain procedure used to test them that I could follow for D8?).