Bugzilla – Full Text Bug Listing
| Summary: | Provide out-of-the-box monitoring of UI delays | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Marcus Eng <marcuseng23> | ||||||||||
| Component: | UI | Assignee: | Lars Vogel <Lars.Vogel> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | daniel.rolka, daniel_megert, eclipse.sprigogin, het, Lars.Vogel, mober.at+eclipse, pwebster, robert.munteanu, tparker | ||||||||||
| Version: | 4.4 | ||||||||||||
| Target Milestone: | 4.5 M2 | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Bug Depends on: | 360052 | ||||||||||||
| Bug Blocks: | 442335 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Marcus Eng
Sergey, I think you created a Watchdog plug-in test plug-in in the past. Would that be fitting for this request? And if, could you contribute it? (In reply to Lars Vogel from comment #1) Marcus is an intern in our group who's project was to open source our monitoring of UI delays. Stay tuned. (In reply to Sergey Prigogin from comment #2) > Marcus is an intern in our group who's project was to open source our > monitoring of UI delays. Stay tuned. Cool, looking forward to his Gerrit review. https://git.eclipse.org/r/#/c/31002/ I've posted a patch of what has been made so far. Can someone please review it? Right now, it currently includes a feature, but we prefer it to be part of Platform UI. Looks great Marcus, maybe the ui for the UI delay and Sample data could improved? It does not look to me as if the stacktrace will be ever filled on this level, confused me at the begining. Maybe don't show the stack trace part of the UI on this level? Or can it happen that it is filled here? I also think that this monitoring should be turned of by default. I assume otherwise the Eclipse users will get scared. :-) Also "Fully Qualified Methods Names of Stack Traces to Filter" is a bit hard to parse. Maybe give a better description similar to the "Type Filters" preference setting? (In reply to Marcus Eng from comment #4) > Right now, it currently includes a feature, but we prefer it to be part of > Platform UI. Will the Google team maintain this plug-in and work on issues reported with it? (In reply to Lars Vogel from comment #5) > Looks great Marcus, maybe the ui for the UI delay and Sample data could > improved? It does not look to me as if the stacktrace will be ever filled on > this level, confused me at the begining. Maybe don't show the stack trace > part of the UI on this level? Or can it happen that it is filled here? > > I also think that this monitoring should be turned of by default. I assume > otherwise the Eclipse users will get scared. :-) > > Also "Fully Qualified Methods Names of Stack Traces to Filter" is a bit hard > to parse. Maybe give a better description similar to the "Type Filters" > preference setting? Hi Lars, Thanks for your feedback. The ui for what you mentioned is a bit confusing at first. The reason why we do not move the stack trace up one level to the "Sample at" is because there can be multiple threads (and, consequently, multiple stack traces) logged under a single sample if the "Dump All Threads" setting is enabled. And to your comment regarding it being an API and x-internal, we created an extension point and utilize it for logging to our own remote servers, which is why we need an API. I did adjust the code regarding your other suggestions. Thanks! Marcus (In reply to Lars Vogel from comment #6) > (In reply to Marcus Eng from comment #4) > > Right now, it currently includes a feature, but we prefer it to be part of > > Platform UI. > > Will the Google team maintain this plug-in and work on issues reported with > it? Yes! The Google team will (In reply to Marcus Eng from comment #8) > Yes! The Google team will Awesome. This week we have M1 test freeze, but I try to give it a detailed review after M1 and if no one objects, integrate it into platform.ui. Dani, Paul, this is really useful IMHO. Any objections against adding to to platform.ui? Created attachment 245929 [details]
Full screenshot
I've included some screenshots of the plug-in in use
Created attachment 245930 [details]
Error log printout
Created attachment 245931 [details]
Plug-in preference page
Hi, I wanted to know if it were possible to have this reviewed and merged before the end of this week. I am an intern, and my last day is this Friday (8/15). Thanks! Marcus (In reply to Lars Vogel from comment #10) > Dani, Paul, this is really useful IMHO. Any objections against adding to to > platform.ui? Do you want to review it? I'm +1 for merging it, I tested it and its works fine. I even found a nasty performance bug in e4 tools (still need to fix it) with the tool. Created attachment 246012 [details]
Error log printout
We cleaned up the printout in the error log a bit.
I was wondering if there was any feedback regarding the patch and if it were possible to have this merged into platform.ui before the end of tomorrow. I apologize for the pings but tomorrow is my last day. Thanks again, Marcus (In reply to Marcus Eng from comment #17) > I was wondering if there was any feedback regarding the patch and if it were > possible to have this merged into platform.ui before the end of tomorrow. Actually that is a good reason NOT to apply it. You mentioned earlier Google will be continue to maintain it, who would be the contact person in case we find later issues with the development? Daniel, Lars, Paul and I discussed this offline. Here's the outcome: - I will support this feature being added to the Platform and hence will give the PMC +1 on the CQ. Yes, this contribution exceeds the threshold and needs a CQ being approved by the PMC and the Eclipse legal team. Therefore it is impossible to be committed at this point. - Lars volunteers to be the buddy of this feature. This does not mean that he owns it. Lars and I expect that someone from Google steps forward as the owner and Lars being the one who reviews and commits the code with due diligence. I took a really quick look (i.e. don't treat this as my review) and have the following comments: - should not cause any new warnings in the workspace - bundle-version constraints missing - API tools setup is not done - API tools tags missing entirely - @since tags missing - we use single quotes for quoting - too much API (should be exposed as interfaces) - PreferenceConstants not static final - @since tags missing - Preference page - maybe move under Tracing - dependent items need to be indented and disabled - missing mnemonics everywhere - New -> Add - enabling of OK button has troubles - we should probably allow to log into a separate file (In reply to Dani Megert from comment #19) > Lars and I expect that someone from Google steps forward as the > owner and Lars being the one who reviews and commits the code with due > diligence. I volunteer to maintain this feature going forward. (In reply to Sergey Prigogin from comment #20) > I volunteer to maintain this feature going forward. Great to hear. Can you and Marcus start addressing the issues Daniel pointed out? I start the CQ. (In reply to Lars Vogel from comment #22) > CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=8571 Please add another author, Steve Foreman, Google, to the CQ. Thanks. (In reply to Dani Megert from comment #19) > - should not cause any new warnings in the workspace Done > - bundle-version constraints missing Done > - API tools setup is not done Done now > - API tools tags missing entirely I don't see any places where API tools tags would be appropriate. Please advise. > - @since tags missing I wasn't sure if @since tags were appropriate in the initial version. @since 1.0 is now added to the classes/interfaces in org.eclipse.ui.monitoring package although the package is exported as x-internal following a Lars' advice. > - we use single quotes for quoting Done > - too much API (should be exposed as interfaces) The only two non-essential APIs are constructors of UiFreezeEvent and StackSample classes. Hiding these constructors behind interfaces looks like an overkill. Please let me know if you disagree. > - PreferenceConstants not static final Added a private constructor. All fields are static final. Do you think that the class should be declared final too? > - @since tags missing See above > - Preference page > - maybe move under Tracing Moved under Tracing although the feature is intended for regular users as a tool to file quality bugs, not just for plugin developers. It would be nice if the preference page were more visible than Tracing. > - dependent items need to be indented and disabled Dependent items are now disabled properly. Indentation would look weird since everything except the first checkbox would be indented. All existing preference pages of similar structure don't have indentation, only disablement. > - missing mnemonics everywhere Fixed. > - New -> Add Renamed to Add &Filter. Simple "Add" is problematic due to collision with mnemonics of &Apply and Restore &Defaults > - enabling of OK button has troubles I wasn't able to reproduce any problem with enablement/disablement of the OK button. What are the steps to reproduce the problem? > - we should probably allow to log into a separate file Logging to a separate file would mean loosing the power of the Error Log view and making the feature less accessible to average Eclipse users. Unlike tracing, UI freeze events have a well-defined structure that is nicely shown in the Error Log view. If you meant adding logging to a separate file as an extra option in addition to ability to log to the standard error log, that is definitely possible, but introducing it now seems premature. Are you afraid of Eclipse responsiveness being so bad that the error log would be flooded by UI freeze events? IMHO we should use the normal log file, IIRC there is a GSoC project which publishes the logs at a server. Freeze data would be very useful. (In reply to Sergey Prigogin from comment #23) > (In reply to Lars Vogel from comment #22) > > CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=8571 > > Please add another author, Steve Foreman, Google, to the CQ. Thanks. Isn't Marcus Eng the original author regarding legal approval of the CQ? (In reply to Dani Megert from comment #26) > (In reply to Sergey Prigogin from comment #23) > > (In reply to Lars Vogel from comment #22) > > > CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=8571 > > > > Please add another author, Steve Foreman, Google, to the CQ. Thanks. > > Isn't Marcus Eng the original author regarding legal approval of the CQ? The original Google-internal version of the code was written by Steve Foreman. Marcus Eng extended it and made it suitable for open-sourcing. The copyright headers of all files list the corresponding contributors. I updated the Gerrit review with some minor correction suggestions, afterwards I plan to integrate it into platform.ui. Any other concern which others may have can be solved later via new bug reports, I would like to have that in M2 to get broader feedback. Awesome work Sergej and Marcus. I submitted the current version and we can work on the remaining issues in separate bug reports. Thanks for doing this work, I think this monitoring can be a key part in making the Eclipse IDE really responsive. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=9b4722b71f6463402aa8d0cd490370682952288a I really like this - great work ! I just have one question: From the descriptions I've read so far, the delay monitor logs the stacktrace of the UI Thread in case of a delay. But in many cases (that at least I have seen, and certainly in case of compleate deadlocks) other Threads are involved in the issue. So looking at the UI Thread only won't help much. Wouldn't it make sense to collect other Threads too, or create a complete Thread Dump ? Or is this already done and I have overlooked it ? Hi Martin, You have the ability to dump all the threads already! If you go to the preference page there's a checkbox to "Dump All Threads" Marcus (In reply to Marcus Eng from comment #31) > Hi Martin, > > You have the ability to dump all the threads already! If you go to the > preference page there's a checkbox to "Dump All Threads" > > Marcus Regardless of whether "Dump All Threads" is enabled, the DeadlockTracker class in http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org.eclipse.ui.monitoring/src/org/eclipse/ui/internal/monitoring/EventLoopMonitorThread.java can be updated to grab all threads when a deadlock is being reported and log that stack sample. That would be a nice enhancement. Unfortunately not yet in our build result. I updated comments for StackSample and UiFreezeEvent. https://git.eclipse.org/r/#/c/33410/ (In reply to Terry Parker from comment #34) > I updated comments for StackSample and UiFreezeEvent. > > https://git.eclipse.org/r/#/c/33410/ Thanks Terry. Merged with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c39ab88756bc8d2562eae2b903ca7225ccf5d593 |