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

Optimize file reading #202

Closed PhRX closed 7 years ago

PhRX commented 7 years ago

The FileLogSource was significantly slowed down by redundant FileChannel size checking. Also, increasing the buffer reduces the size checking even more. This fix improves performance by several orders of magnitude.

nitsanw commented 7 years ago

Once your PR has been accepted you can rebase the incremental branch to pull in changes from upstream. This is the right way IMO.

PhRX commented 7 years ago

So (from my perspective) :

  1. Create branch off of forked master
  2. Make changes
  3. Issue PR
  4. Merge branch into forked master
  5. Wait for PR Acceptance (merge into RW/master)
  6. Prof^H^H^H^H Rebase forked master based on RW/master

Is that the correct workflow then ?

PhRX commented 7 years ago

Oops sorry wrong button, closed PR by accident.

nitsanw commented 7 years ago

You don't need to merge twice if you are willing to wait. Small PRs can be approved quickly, you should try it ;)

PhRX commented 7 years ago

OK so what do I do with this one - hard reset, put it on a branch off of https://github.com/PhRX/honest-profiler-fx-ui/commit/09048341f6e2770c588cc2bc7ab35b8f784fb17a (AFAICS the root of teh first open PR) and resubmit a new PR ? Or just leave it for now ?

nitsanw commented 7 years ago

You rebase your master branch to upstream, create a branch off it, apply a small change, push and PR.

nitsanw commented 7 years ago

Alternatively, you wait for all your outstanding PRs to be accepted, you can always do that. Personally I would like you to stop making mega PRs. I am particularly concerned that amidst the javafx work you are doing which is a rewrite of a UI component there are also core changes slipped in that are under reviewed because of the inclusion. I would highly recommend splitting ANY core change into a separate PR.

PhRX commented 7 years ago

These PRs were supposed to be small - I just didn't know up-front that they'd show up as cumulative (I originally expected only the commits in the particular branch to show up). It should be clear comparing the PRs between them that I've started compartmentalizing the changes. This last one only contains a core fix, for instance - I'm well aware of the need to clearly separate core fixes from the resr.

Now I just need to tune my git workflow so that I can submit parallel PRs without being forced to wait on reviewers' acceptance before being able to benefit from the changes myself, and without having to merge unduly.

PhRX commented 7 years ago

I've redone the fix, this time having turned off all auto-formatting to minimize the diff.

@nitsanw I've been thinking about your rebase advice and reading up on git, but whichever way I look at it, it seems that the history would just get overcomplicated. I'll just wait, I don't have anything else in the making for now anyway.

I'll ensure that, if the current batch of PRs get accepted, afterwards all PRs will be parallel. I'm still not entirely convinced that that is better in case 2 parallel PRs touch the same file (since that generates more complicated merges for everyone, hence more scope for errors) but since, as I've said elsewhere, most of what I need from the profiler front-end is now there, big changes from me are getting less likely anyway.

nitsanw commented 7 years ago

I have no intention to discourage contribution, but I think in this case there is no reason for the PRs to not be parallel. I would like to rework HP repository structure/breakdown to make the different parts of the project easier to maintain and develop going forward, so that core/ui changes are never mixed in one PR in any case.

PhRX commented 7 years ago

Superseded by upcoming, cleaner PR.