RFR: 6531: Make the flame chart view render labels on windows
Alex Macdonald
aptmac at openjdk.java.net
Mon Feb 24 20:38:13 UTC 2020
On Mon, 27 Jan 2020 17:00:14 GMT, Marcus Hirt <hirt at openjdk.org> wrote:
>>> It doesn't feel right to me to maintain d3-flame-graph fork, as @thegreystone stated it's a bit scary.
>>
>> As for "maintaining the fork", the next version up from the fork (v2.0.3) is incompatible with IE, so the fork is basically a one-shot fix that can't be updated. The interesting thing here is that we're using v2.0.3 (Sept 2018) , so if we had used just one release prior then the flame labels would have rendered in JMC Windows (without text) prior to the latest commits for colouring.
>>
>>> I'd propose the way @thegreystone has stated -> SWT update to use an alternative (newer) browser.
>>
>> Chromium support was initially slated for integration Q1 2017 [0], and the related patch looked to be on track to be posted by the end of 2019 [1], but it hasn't made it out yet. So when the patch does get posted, it'll need review, inclusion into SWT, and then shipped with a release .. we're looking at potentially quite a bit of time before this happens, but hopefully not.
>>
>> I'm conflicted both ways, so I'll go based on the consensus here. I'm not sure how popular JMC is for Windows, or how many people are wanting (or trying) to use the flame view, but it doesn't sit right with me that the view doesn't display anything. Maybe it can print an error screen mentioning the incompatibility instead?
>>
>> [0] https://projects.eclipse.org/development_effort/implement-swtchromium-integration
>> [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=549585#c20
>
> I think I'm ok with this being a stop-gap measure. It does make third party-dependencies a bit funny - and there is a bit of trust involved in getting the javascript from a private fork of a project, and even a private push of the resulting build to a CDN. As soon as the Eclipse changes are in, in any form and shape, we should switch.
> As soon as the Eclipse changes are in, in any form and shape, we should switch.
Agreed, it would be very limiting to continue on this way.
This PR required some rebasing after the latest flamegraph commits to include colour highlighting. I had to include an additional commit here that allows for the colouring to work with IE, because some syntax and functions are too advanced for ES2015.
About the `flameviewColoring.js`, I made the changes in the file to make it compatible with IE, but would it be worthwhile to go the route of making a `flameviewColoring-ie.js` so that if/when Chromium functionality is introduced instead of refactoring the colouring script we could just drop the `-ie` version? I guess this only makes sense if the flamegraph enhancements are mostly finished, otherwise it'd be a burden to upkeep two scripts.
Here's an example screenshot of it working in Windows with text highlighting:
-------------
PR: https://git.openjdk.java.net/jmc/pull/41
More information about the jmc-dev
mailing list