Feature Bundle

[PhRX]

This PR supersedes #173, which pretty much is obsoloete given the volume of changes.

It almost concludes the work I wanted to do. An overview of the changes can be found below.

Since the amount of code is probably larger again than what's in #173, reviewing isn't getting any easier. I'm aware of this. but IMHO having to wait for reviews should not hold up progressing on the project if the progress is on a large enough scale.

I'd like to propose the following procedure :

• Create a new branch in the original HP repository called "approval"
• Close the 2 PRs (this one and #173)
• Let me create a new PR pointing at the "approval" branch
• Merge the PR into "approval"
• In the instructions, make people aware there's an alternative approval branch which has more features
• Let people use it for a while; if possible, also review the code
• If possible, have somewhere where people can vote for/against the "approval" changes
• If enough people agree that it works and is a worthwhile evolution, and any bugs or issues found by users have been fixed, integrate the branch into master

This way, we add an alternative standard for PR acceptance next to code review : enough users being happy as yardstick. Also, a branch on master is more visible than "just another fork".

Please let me know whether that works for y'all.

So, in addition to what's in #173, the following features have been added :

• Fix for #36 : the aggregation options (see below) allow collapsing thread trees into an aggregated tree
• Fix for #38 : the aggregation options allow inclusion of the line nr, which disambiguates the method calls (but having the signature somehow would be even nicer)
• Fix for #78 : during "live" profiling, the profile can be "frozen", so the front-end no longer receives updates. When unfreezing, the view will be updated to the current situation.
• Fix for #175 : large log files can now be opened. The log files are no longer memory-mapped in their entirety, instead the buffer now is a "moving window".
• Fix for #179 : top-level frames (children of thread-level) are now aggregated correctly
• The Self Time and Total Time, available in the agent output, is now exposed in all views
• Aggregation can now be switched as follows :
  • Frames : By Fully Qualified Method Name, By FQMN + Line Nr, By FQMN + BCI
  • Threads : All threads together, Threads by Id (tree views only), Threads by name (tree views only)
• The source profile (LeanProfile) now contains the minimum of information without redundancy, to ensure the collection thread has to do as little work as possible
• Pull mechanism for profiles : the front-end now requests updates, which allows it to control the update rate (but no UI controls have yet been provided except for the "freeze" buttons)
• Aggregation happens lazily and is cached
• Tree Diff view for comparing two profiles
• Added Infobar with help information
• In the Flat view (list of methods), when selecting a method, its ancestors (methods directly or indirectly calling the selected method) and descendants (methods directly and indirectly called by the selected method) are shown in two trees at the bottom of the screen
• In the Tree view, when selecting a method, its descendants are aggregated into a list shown at the bottom
• Added more columns in all views, with homogenous labeling
• Added QuickFilter function for filtering the key
• Prevent opening the same source twice
• Fixed VirtualMachine source issue where the log file produced by an agent couldn't always be located
• Included font with non-proportional digit characters for more readable number tables
• Smaller default font size (through CSS)
• On refresh, expand TreeViews by 1 level
• Sorting is preserved (at least in flat views) when the aggregation is updated
• I18N for all text (except some enum choices, still got to figure out the pattern for that)
• Remember parent directory when opening a log file, to prevent lots of renavigating to the same or similar location

At code level, the following was changed :

• New core.aggregation package added, containing :
  • aggregator package with the actual aggregation transformation logic
  • filter package with more generalized filtering logic
  • grouping package with "key grouping" logic for aggregation
  • result package with the aggregation data structures
• New core.collector.lean package containing the new LeanLogCollector which implements the pull mechanism and which produces the more lightweight "LeanProfile"s
• New core.profiles.lean package containing the data structures for the new "lean" profiles
• Extracted common functionality from controllers and put it into abstract superclasses for code reuse
• Use JavaFX tasks to decouple profile generation and initial aggregation from front-end
• Documented many methods and classes (but some documentation eneds to be revisited due to recent fixes and refactorings)

Still to do, optionally :

• Percentages are now calculated relative to the "global aggregation", over threads. Allow reference switching to thread-level where appropriate (i.e. in tree views aggregated per thread, show percentages relative to thread total rather than relative to sum over all threads)
• In addition, maybe add option not to take the "error" counts into account when aggregating ?
• Highlight filter hits (useful in tree filtering)
• Maybe add aggregated ancestors/descendants views to flat diff and tree diff ?
• Make tabs/profiles closeable
• Add Preferences, including styling options (font size, ...)
• Clearer QuickFilter UI ? (show currently active quickfilter, to avoid confusion when something was typed in the text field but not "activated")
• Add Clear QuickFilter button / Clear All Filters button
• Remember selection when aggregation is updated
• Remember which tree nodes were expanded/collapsed when aggregation is updated

[RichardWarburton]

Hi @PhRX ,

Thanks for continuing to work on this stuff. I don't really like the "approval" branch idea, sounds like it'll cause a lot more confusion. Better to just progress with the review. Since this PR is more advanced than the last one then I'm happy to have a go at reviewing this one. I propose we close #173 and focus on this one. What do you think?

[PhRX]

Thanks @chhsiao90 and @nitsanw for the review.

@RichardWarburton I forgot to reply, but I agree (and I already closed the old PR), although I'd like to do something about the visibility of the new stuff - a fork is in my (limited) experience very easily ignored entirely.

I'll finish up the documentation - only some utility classes are undocumented, and I'll probably write some package documentation which should make it clearer how the concepts work together - I can imagine that things like Grouping and Target may be a bit confusing or obscure when first coming across them. Some pictures or at least some text with a top-down explanation might clarify (comments in the code are by nature more bottom-up).

[PhRX]

To all potential reviewers : in my most recent commit I added the package description for the core.profiles.lean package.

I very very highly recommend reviewing it early on, since I tried to describe in detail the profiler basics, the internal structure of the LeanProfile data structure (which the core data structure on top of which all aggregations are built), and how the numerical data in there is (minimally) aggregated.

As a result, the text pretty accurately reflects many of my presumptions and assumptions when I wrote the code. Therefore, if there are any conceptual mistakes in this text, it is fairly certain that those mistakes are reflected in the code !

Unfortunately for me the converse is not true - if the text is accurate and sound, it doesn't mean the actual code is correct :-/

Of course an additional benefit of reading it is that the LeanProfile structure should be clearer afterwards.

[RichardWarburton]

Hi,

I've tried out the new JavaFX UI and in general it looks good. I like the multiple profile tabs and the comparison view - good improvements.

I've noticed a couple of things that look like regressions:

• If you start another Java program, it will appear in the monitor menu, but if you quit the other program then its entry won't disappear from the menu.
• If you file->quit or click X in the ui after you have opened a log it closes the window, but doesn't actually exit the Java process.
• When I try to monitor a program I get the following exception:

java.lang.StringIndexOutOfBoundsException: String index out of range: -11
    at java.lang.String.substring(String.java:1967)
    at com.insightfullogic.honest_profiler.core.sources.VirtualMachine.getLogSourceFromVmArgs(VirtualMachine.java:73)
    at com.insightfullogic.honest_profiler.ports.javafx.model.task.InitializeProfileTask.monitor(InitializeProfileTask.java:80)
    at com.insightfullogic.honest_profiler.ports.javafx.model.task.InitializeProfileTask.call(InitializeProfileTask.java:45)
    at com.insightfullogic.honest_profiler.ports.javafx.model.task.InitializeProfileTask.call(InitializeProfileTask.java:22)
    at javafx.concurrent.Task$TaskCallable.call(Task.java:1423)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

[PhRX]

Hi @RichardWarburton,

Thanks for the testing ! I addressed the 3 issues you mentioned :

• The JVM Stop not being detected seems not to have been a regression, but an issue in the "original" code : it would always try to attach a VM also in the "close" case. "attach" is now called only once as well (which addresses the TODO comment in there, which I now realize I forgot to remove). Not sure if there is a case for "remembering" descriptors after a failed attach, and trying to attach again on subsequent polls ?

• The Java process would exit eventually, but I made it snappy by ensuring the executor service in the application context gets shut down on application close, and that LocalMachineSource doesn't ignore being interrupted but triggers the ThreadedAgent interrupt logic properly.

• Are you sure that you're running the latest version from the integration thread ? The line numbers in the exception didn't match up with mine, and in https://github.com/RichardWarburton/honest-profiler/pull/184/commits/54f79108f2bf2aec442889ce0fa1bf5e13171e7b I hardened the "logPath extraction" code quite a bit (precisely to avoid the OutOfBounds exceptions like you're seeing). I reviewed the code again, and added a minor fix, but that corner case (the args containing "logPath" but not "logPath=") should not have applied to your test.

[PhRX]

@RichardWarburton @chhsiao90 I was very focused on finishing the documentation yesterday, I didn't have time yet to remove the gradle bits (again). Will do so shortly.

@chhsiao90 and others : Is the landing page important ? Which functions or information would you like to see there ? It's not very difficult to reinstate but its functions have effectively been superseded by the menu options, and at the moment, once a profile has been opened, the landing page would never be seen again (because I haven't yet implemented any profile closing functions). That said, while mulling this yesterday I did realize the initial view looks a bit early 90s :-) (i.e. unwelcoming big blank area of nothing).

Suggestions and feedback more than welcome !

[chhsiao90]

@PhRX - landing page is not very necessary and seems not import with this PR. I think keep empty in current version is fine :)

[RichardWarburton]

Hi @PhRX re: "Are you sure that you're running the latest version from the integration thread?" - I cloned a repo from your PR and used the compiled binary from that to produce the log. It was the latest commit as of the comment. I'll re-test this evening with your latest pushes into this PR.

[PhRX]

Hi @RichardWarburton I didn't even know you could clone from a PR. If you want me to, I can update the code to collect more information and make the Exception describe precisely what went wrong. I just re-eyeballed the current state and I don't see anything anymore that could cause trouble - either the agent was attached to the JVM with -agentPath + a logPath arg, in which case the file described by logPath will be returned, otherwise the old code is invoked.

Thanks for testing !

[RichardWarburton]

After both using the build of this for a while and talking to @nitsanw I've decided the best thing is to merge this PR and cleanup any remaining issues afterwards. But please, please, please send single-purpose PRs in future!