Feature Bundle #184
Feature Bundle #184
Conversation
* Eclipse plugin : execute "gradle eclipse" to generate an eclipse project * Application plugin : execute "gradle run" to start the JavaFX UI
one contains the name. Instead of creating new ThreadMeta objects in the profile, update the existing one if it has "more accurate info (i.e. a non-empty threadName).
…he first" This reverts commit a830cb3.
[Corrected with spaces instead of tabs] Multiple ThreadMetas are emittled for the same thread but only the first one seems to contain the ThreadName. So instead of putting the last seen ThreadMeta in the profile, the ThreadMetas are merged.
This reverts commit 7b48244. Seems that local git settings were not auto-converting the line endings yet.
[Corrected with spaces instead of tabs] [Corrected CRLF settings] [Formatting as close to original as I could get it with Eclipse formatter settings] Multiple ThreadMetas are emittled for the same thread but only the first one seems to contain the ThreadName. So instead of putting the last seen ThreadMeta in the profile, the ThreadMetas are merged.
Using the same formatter settings for the project allows contributors to keep formatting consistent. This one was exported, after configuration, from Eclipse Neon 4.6.2. It is as close to the original formatting as I could get it. Any improvements welcome.
The Flat Profile Entries now contain percentages, which are already an aggregation. This makes later aggregation (e.g. in the UI) much more difficult. The self/total sample counts are the "raw data", and can later be aggregated in different ways (e.g. show how much time is spent in a particular package). The traceCount has been added because there's no link (yet) to the total sample count. This may need to be reworked, with the total sample count being kept at Profile level, and aggregation to percentages taking place there.
…m-profiling-tools#78) The Profile.copy() method allows creation of a deep copy. This decouples the profile publication from processing on the output (ui, console, ...) side. This means for instance that filters in the UI can be applied on a copy, and when the filter is removed, the original profile can be reinstated. The copy also can fulfill the role of a snapshot (i.e. for implementing jvm-profiling-tools#78).
Calculate the septh of the descendant tree of a ProfileNode.
New StringFilter superclass with implementations for filtering on classname and on method name. There's also support for more filtering modes : contains, startsWith, endsWith, ...
This is the first step of a big refactor + functionality leap. The step is split out in order to ensure git properly keeps track of the change history.
Renaming the Controllers to [something]Controller. Separate step to allow git to track rename easily.
More moving to different packages, allowing git to track easily.
Move FXML files to a subfolder called "fxml", making room for other subfolders for css and icons.
Add new resources : icons and css
Add Utility classes used in upcoming functionality. TimeShareFilter extended with more comparison features.
Bring FXML filenames in line with Controller name pattern
List of functional changes :
- Allow opening multiple profiles
- Instead of Landing View, open log files or machines from Menu Bar
- Use Choice Box for selecting views
- Specify filters using UI instead of a string description
- Extended filter capabilities :
* Also filter on Method Name
* Added several comparison operators
* Allow hiding threads which contain only "error" frames
(works only in Tree View)
- Changing filter or removing filter will reapply filters to
"original" profile which was filtered
- Collapse All / Expand All buttons added in Tree View
- Right-click on node in tree view for more expand/collapse options
on the node itself
- Allow creating a diff of the flat views between two profiles
(keyed by method name)
* Diff view shows base and new numbers, and highlighted difference
- Export flat view or diff view as currently shown (i.e. filters
taken into account) to a CSV file (using export button)
- Export (part of) a stack in tree view to a text file by
right-clicking the node
- Show Thread Names in Tree View when known
- Show sample counts as well as percentages
mousing over controls
The result is a more compact and more easily interpretable view. Also, an icon is added to show whether the profile comes from an HPL file or is "live"
The VM names in the Monitor menu and elsewhere can be way too long because of extra java options in the process name. Shortened to first occurrence of space character.
Since my personal use case is mostly offline processing of hpl log files, the "live monitoring"option hadn't been tested yet. Since on testing the heuristics apparently can't locate the hpl file for a live vm on my machine, and since the "live" mechanism actually also reads from a "live" log file, the new menu item allows you to open a "live" log file, and that works nicely. For now I left the "monitor" menu, in the hope of finding a way to locate the log file produced by the agent somehow.
There was a problem hiding this comment.
Just think it would be better if more people help review on this awesome PR!
Please see a few comments in the code.
The PR had two major modification, one is LeanProfile, and other is gui improvement.
The gui part I think it was great, I was enjoyed at the fancy user interface!
But if there is a start page, that would be better!
For LeanProfile part, I think it still need some times to find potential bugs or typos.
But the code was awesome!
BTW, Gradle stuff appeared in this PR, I think it was still better to be removed for #173 (comment)
| this.className = className; | ||
| this.methodName = methodName; | ||
| } | ||
|
|
There was a problem hiding this comment.
Just a suggestion.
Is that modify the original constructor to private and create another public method Method construction will be more readible?
public static Method of(long methodId, String fileName, String className, String methodName)
{
return new Method(methodId, fileName, formatClassName(className), methodName);
}
private Method(long methodId, String fileName, String className, String methodName)
{
this.methodId = methodId;
this.fileName = fileName;
this.className = className;
this.methodName = methodName;
}
There was a problem hiding this comment.
out of scope, lets not add further changes to this already massive PR.
There was a problem hiding this comment.
Thanks for helping with review this PR @nitsanw and @chhsiao90 - greatly appreciated! I still think we should keep to one build system and not have it as part of this PR. I don't mind if we move to gradle in future, but let's not bundle that up in these changes.
| } | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
I think it still can keep this class' immutability as following.
immutable still better than mutable!
public ThreadMeta update(ThreadMeta newMeta)
{
if (newMeta.threadName != null && !newMeta.threadName.isEmpty())
{
return newMeta;
}
return this;
}There was a problem hiding this comment.
Fixed in upcoming commit, along with ThreadInfo which follows the same pattern.
| public LeanNode copy(LeanNode newParent) | ||
| { | ||
| return new LeanNode(this, newParent); | ||
| } |
There was a problem hiding this comment.
Not very straightforward for the method name copy.
Because it didn't just copy, but also set a new parent for it...
It will be better if it had a more proper name, but still acceptable for me :)
There was a problem hiding this comment.
Fixed in upcoming commit, by getting rid of the two copy() methods. One was unused, and the one referenced here was used only in a single location where it didn't hurt readability to replace it by a constructor call.
| * Copy constructor. | ||
| * | ||
| * @param source the source LeanNode which is being copied | ||
| * @param new Parent the parent of the copy (which itself generally is a copy) |
There was a problem hiding this comment.
Fixed in upcoming commit.
| // Aggregation Methods | ||
|
|
||
| /** | ||
| * Aggregates teh information from a child {@link StackFrame} into the children of this LeanNode, updating the total |
There was a problem hiding this comment.
Fixed in upcoming commit.
| /** | ||
| * Sets the {@link ApplicationContext} and propagates it to any contained controllers. | ||
| * | ||
| * @param appCtx the {@link ApplicationContext} |
There was a problem hiding this comment.
appCtx => applicationContext
There was a problem hiding this comment.
Fixed in upcoming commit.
| int logPathEnd = commaPos > 0 ? commaPos : (spacePos > 0 ? spacePos : agentPath.length()); | ||
|
|
||
| return new FileLogSource(new File(agentPath.substring(logPathStart, logPathEnd))); | ||
| } |
There was a problem hiding this comment.
I think this part had three problem that cause my hp-gui cannot attach to a running jvm correctly.
- The jvm only had -agentpath as vmArgs that will cause
ArrayIndexOutOfBoundsException
java -agentpath:/Users/han-pc/IdeaProjects/honest-profiler/build/liblagent.dylib=interval=100 -cp classes InfiniteExample
- The jvm didn't defined custom logPath
java -agentpath:/Users/han-pc/IdeaProjects/honest-profiler/build/liblagent.dylib=interval=100 -cp classes -Dwhatever=Y InfiniteExample
- The jvm define logPath with relativePath, but was running on different folder with hp-gui
java -agentpath:/Users/han-pc/IdeaProjects/honest-profiler/build/liblagent.dylib=interval=100,logPath=tmp.hpl -cp classes -Dwhatever=Y InfiniteExample
There was a problem hiding this comment.
Are these problems not present with previous version?
There was a problem hiding this comment.
No,
previous version seems only used getLogSource() to get latest log file at userDir
And current version used getLogSourceFromVmArgs() depends on vmArgs
There was a problem hiding this comment.
Hm, my bad, I'll check and fix. The intention was to revert to the original logic if no logpath parameter was found, so as to only extend the number of cases in which the mechanism works correctly.
There was a problem hiding this comment.
Fixed in upcoming commit. I hardened the code, by introducing early checks if what we're looking for is present at all, and revert to original getLogSource() if it isn't. Also, the final File is checked for existence, and if it doesn't, revert to the original getLogSource() as well.
| private BigInteger totalTime; | ||
|
|
||
| private int selfCnt; | ||
| private int totalCnt; |
There was a problem hiding this comment.
Is that selfCnt or totalCnt had the risk for overflow?
int -> BigInteger will be better?
There was a problem hiding this comment.
Given that this is a number of samples, and that INT_MAX is rather large, it would take a program which samples 1000 times a second nearly 50 days of profiling to overflow. I would not worry.
There was a problem hiding this comment.
Thanks @nitsanw for doing the calculations for me :) This is pretty much what I intuited, when deciding between long and int (BigInteger would have been overkill), but I never actually did the math. The only reason for the BigInt is that when working with nanos, you potentially do get really big numbers.
- Several javadoc typos fixed - Made ThreadMeta and ThreadInfo immutable - Removed copy() methods from LeanNode - Cleaned up and hardened VirtualMachine LogSource generation, reverting to original logic if no agentpath or logPath parameters are defined in the JVM args
|
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). |
| * {@link TraceStart} following it | ||
| * @param child the child {@link FrameInfo} of the current LeanNode | ||
| * @param last a boolean indicating if the child is the last in the stack trace sample | ||
| * @return this node |
There was a problem hiding this comment.
Is that the method return this node, or return child node !
There was a problem hiding this comment.
Fixed in upcoming commit.
| v.addSelf(nanos) : | ||
| // Child is not last in stack, return existing child (its "total" numbers will be updated when its | ||
| // child is added) | ||
| v)); |
There was a problem hiding this comment.
This part is a little complex...
Can I simplify this logic with following description about this part.
- create child LeanNode if childMap absent
- addSelf if last
The code will turn into following code snipped if I'm correct...
public LeanNode add(long nanos, FrameInfo child, boolean last)
{
// Non-self add, which updates total time and sample count only.
data.add(nanos, false);
LeanNode childNode = childMap.computeIfAbsent(child, k -> new LeanNode(k, this));
if (last) {
childNode.addSelf(nanos);
}
return childNode;
}
What do you think!
There was a problem hiding this comment.
Yes, excellent catch ! This is indeed a lot simpler. As a bonus I can get rid of the "self constructor" as well, as well as a now obsolete constructor in NumericInfo.
Thanks !
Fixed in upcoming commit.
| { | ||
| // Class Properties | ||
|
|
||
| private static final long SECONDSTONANOS = 1000 * 1000 * 1000; |
There was a problem hiding this comment.
SECONDS_TO_NANOS would be better :)
There was a problem hiding this comment.
Fixed in upcoming commit.
- Fixed javadoc typos - Better constant name - Simplification of LeanNode aggregation code
I'm not adding comments to the Cell and TreeItem subclasses in the ports.javafx.view package, they are by and large too trivial. Also, I'm bored stiff after almost two days of writing javadoc comments. But it is a useful exercise nonetheless, you end up reviewing all of your code :)
Otherwise the generated html is all bunched up, since like HTML javadoc doesn't preserve whitespace.
Contains a description of the profiler basics and the LeanProfile structure and aggregation.
|
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. |
The documentation describes how all the aggregation data structures hang together, how aggregations can be created and filtered etc. And the Method Id grouping ? Well I accidentally added a line saying it was supported in the documentation. While reviewing I figured out it wasn't. But it was sooooo simple to add it... Couldn't resist it. Sorry.
|
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:
|
…vm-profiling-tools#184 The code tried to attach a VM descriptor also in the case where the VM actually was closed.
|
Thanks for the testing ! I addressed the 3 issues you mentioned :
|
|
@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 ! |
|
@PhRX - landing page is not very necessary and seems not import with this PR. |
While I don't plan on adding any features to the jvm-profiling-tools#184 PR, this is a simplification which should actually make it easier for reviewers to understand the code, since Dialogs and Dialog Controllers now are handled much the same way as the other ViewControllers, using FXML injection, instead of the roundabout FXMLLoader stuff and explicit Dialog object configuration. Up to now, I never found any resource on the web documenting how to nicely do Dialogs this way. But I finally figured it out, after seeing a couple of sentences buried in https://docs.oracle.com/javase/8/javafx/api/javafx/fxml/doc-files/introduction_to_fxml.html , specifically the remark about needing to nest it in an fx:define. It doesn't help that SceneBuilder also offers no support for this mechanism. Anyway, instead of the ugly explicit FXMLLoader way which you can find in almost any discussion of Dialog creation on the 'Net, which among other things requires explicit FXML paths in the source code, here is now what I strongly suspect is the 'proper' way to do it through FXML controller injection. Yay. And I missed my train because of this.
|
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. |
|
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 ! |
One less file to review ;-)
Back when switching to the new data structures, this fell by the wayside. Now the "Export" function of nodes of (non-diff) treeviews works again. This is my last commit for this PR, apart from potential fixes for bugs or issues reported by reviewers, or if I discover through my own usage any really bad bugs.
|
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! |
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 :
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 :
At code level, the following was changed :
Still to do, optionally :