Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6549: Color flame chart based on package name #29

Conversation

@mirage22
Copy link
Contributor

mirage22 commented Jan 12, 2020

implemented ticket : https://bugs.openjdk.java.net/browse/JMC-6549
additionally:

  1. fixed issue with displaying big data set in flameview
  2. improved display speed

comments: cell coloring algorithm is based on packageName hash with defined depth to avoid collisions and guarantee same stripPackageName color.
conclusion: the flameview is always displayed

Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JMC-6549: Color flame chart based on package name

Approvers

  • Marcus Hirt (hirt - Reviewer) Note! Review applies to 59afc3d
@bridgekeeper
Copy link

bridgekeeper bot commented Jan 12, 2020

👋 Welcome back mwengner! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@openjdk openjdk bot added the rfr label Jan 12, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 12, 2020

Webrevs

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 13, 2020

Also formatting errors. Try mvn spotless:check to see your formatting errors. Usually mvn spotless:apply will fix them for you.

- jmc js : modulo correction
- spotless apply
mirage22 added 4 commits Jan 13, 2020
- adjust gray shades for (java, com.sun, sun)
…JMC_6549_color_flame_chart_based_packagename
- java script file formatting
- javascript file formating corrections
@thegreystone
Copy link
Collaborator

thegreystone commented Jan 13, 2020

Still a few confusing colorings for JDK base packages. We want java., sun., com.sun.* to be grey. (Come to think of it, we probably want to add jdk.* classes to the grey ones too.) And we've said that we want the really core public APIs to be lighter grey and the most internal ones be a darker grey. To me, that means that java.lang should probably be a smidge lighter than java.util. And sun.* be darker than com.sun. We may want to set the colors for these packages, and some other popular ones, like java.io, java.nio ourselves. Or simply have one lighter grey for java., one a little bit darker for com.sun. and jdk., and a dark one for sun.. None with any other hint of color in them (equal amounts of R, G and B).

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 13, 2020

I think the last (easiest) alternative is probably quite okay since:

  1. You typically care less about the JDK classes (you're normally not interested in changing them).
  2. The fact that package transitions in JDK classes are less obvious with the easy pure grey coloring scheme is outweighed by reliably being able to identify them as JDK classes (if it's a pure grey, it's JDK).
@thegreystone
Copy link
Collaborator

thegreystone commented Jan 13, 2020

So, in other words, these simple semantics:

  1. Is it a standard public Java JDK API -> light grey.
  2. Is it a public non-standard Java JDK API -> medium grey.
  3. Is it an internal Java JDK API -> dark grey.
@mirage22
Copy link
Contributor Author

mirage22 commented Jan 13, 2020

I thing that such coloring makes things simpler. I've modified the colors:

  1. standard public (java.*) -> light grey
  2. public non-standard (com.sun., jdk.) -> dark grey
  3. internal (sun.*) -> grey
    scale according to the semantic is from Lighter to Darker (1,2,3)
@mirage22 mirage22 closed this Jan 13, 2020
@mirage22 mirage22 reopened this Jan 13, 2020
- simplified grey scale for  packages : 1. standard public, 2. non-standard public , 3. internal
@mirage22
Copy link
Contributor Author

mirage22 commented Jan 14, 2020

looks like the job has been terminated for windows.

@thegreystone
Copy link
Collaborator

thegreystone commented Jan 14, 2020

Copyrights need to be updated, e.g.:

  • Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.
  • Copyright (c) 2019, 2020, Datadog, Inc. All rights reserved.
mirage22 added 2 commits Jan 14, 2020
- license update
- copyrights update
@openjdk openjdk bot removed the rfr label Jan 14, 2020
@openjdk
Copy link

openjdk bot commented Jan 14, 2020

@mirage22 This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

6549: Color flame chart based on package name

Co-authored-by: Marcus Hirt <marcus@hirt.se>
Reviewed-by: hirt
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 14, 2020
@openjdk
Copy link

openjdk bot commented Jan 14, 2020

@thegreystone Only the author (@mirage22) is allowed to issue the contributor command.

@openjdk
Copy link

openjdk bot commented Jan 15, 2020

@mirage22 Syntax: /contributor (add|remove) Full Name <email@address>

@mirage22
Copy link
Contributor Author

mirage22 commented Jan 15, 2020

/contributor add Marcus Hirt marcus@hirt.se

@openjdk
Copy link

openjdk bot commented Jan 15, 2020

@mirage22
Contributor Marcus Hirt <marcus@hirt.se> successfully added.

@mirage22
Copy link
Contributor Author

mirage22 commented Jan 15, 2020

/integrate

@openjdk
Copy link

openjdk bot commented Jan 15, 2020

@mirage22
Your change (at version 6180dd3) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Jan 15, 2020
@thegreystone
Copy link
Collaborator

thegreystone commented Jan 15, 2020

/sponsor

@openjdk openjdk bot closed this Jan 15, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels Jan 15, 2020
@openjdk
Copy link

openjdk bot commented Jan 15, 2020

@thegreystone @mirage22 The following commits have been pushed to master since your change was applied:

  • 055db4f: 6659: Remove 'Showing X of Y events...' table message when not applicable
  • bfb23cd: 6671: Eagerly convert end time to the same unit as start time

Your commit was automatically rebased without conflicts.

Pushed as commit f74caa8.

@mlbridge
Copy link

mlbridge bot commented Jan 15, 2020

Mailing list message from Marcus Hirt on jmc-dev:

Changeset: f74caa8
Author: Miroslav Wengner <mwengner at openjdk.org>
Committer: Marcus Hirt <hirt at openjdk.org>
Date: 2020-01-15 17:16:02 +0000
URL: https://git.openjdk.java.net/jmc/commit/f74caa81

6549: Color flame chart based on package name

Co-authored-by: Marcus Hirt <marcus at hirt.se>
Reviewed-by: hirt

! application/org.openjdk.jmc.flightrecorder.flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/tree/TraceNode.java
! application/org.openjdk.jmc.flightrecorder.flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/tree/TraceTreeUtils.java
! application/org.openjdk.jmc.flightrecorder.flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/views/FlameGraphView.java
+ application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/jsjmclibs/flameviewColoring.js
! application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template

@mirage22 mirage22 deleted the mirage22:feature/JMC_6549_color_flame_chart_based_packagename branch Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.