search

jvm-profiling-tools / honest-profiler

A sampling JVM profiler without the safepoint sample bias
https://github.com/RichardWarburton/honest-profiler/wiki
MIT License
1.25k stars 146 forks source link

Fix : #188 Flamegraphs #210

Closed PhRX closed 6 years ago

PhRX commented 7 years ago

Fixes broken Flamegraph rendering and adds Flamegraph diffs.

Replaces deleted PRs #192 and #201.

PhRX commented 7 years ago

Commit 58dd0f5 reverts all previous commits from 13de748 up to and including 1e1e31e, so please ignore those commits in the PR commit list.

RichardWarburton commented 7 years ago

This is still a 1600 line PR. Can it be split up to improve readability a little bit?

PhRX commented 7 years ago

@RichardWarburton I reviewed the changes but unfortunately, I don't see any way of splitting this up.

The AbstractFlameCanvas class is a common ancestor for two classes in the only "logical" split that could be made ("diff" Flamegraphs versus "straight" Flamegraphs). If I work on 2 branches, one of the branches would be missing this essential component. And if I'd merge one branch into the other to get this component, again any PR would show all commits of both branches.

I don't see any clear way out of this. I'd be grateful for any advice.

For reviewing purposes, it might help to know that all non-trivial changes are contained to only a couple of classes and files : The aforementioned abstract canvas and its two new subclasses, the two FlameGraph controllers, the Flameblock and the FlameGraph FXML files. Together these 7 files account for > 1K added lines.

RichardWarburton commented 7 years ago

This branch is now in conflict with the master.

PhRX commented 7 years ago

I have absolutely no headroom for looking at this at the moment, I'll try and get to it ASAP.

PhRX commented 6 years ago

Seems like there's an issue with the travis scripts ? Not familiar with all of that so I hope somebody knowledgeable can take a look ?

RichardWarburton commented 6 years ago

I think the travis build should have been fixed by recent PRs into the main repo. So they'll probably start passing if you rebase your branch from main.

RichardWarburton commented 6 years ago

Looks like it passes now.