Bugzilla – Full Text Bug Listing
| Summary: | [TMF] Deadlock in the Events view | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Francois Chouinard <fchouinard> | ||||||||
| Component: | LinuxTools | Assignee: | Francois Chouinard <fchouinard> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | Francois Chouinard <fchouinard> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | bernd.hufmann, jjohnstn | ||||||||
| Version: | unspecified | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Francois Chouinard
Created attachment 185191 [details]
Fix for the deadlock
The fix is simply to remove the event cache filling from the UI thread and have it performed in a Job.
Every time a row has to be updated, the table's handleEvent() is called. If the requested row is cached, then it is simply returned as before. If not, a call is made to populate the cache. There can be multiple simultaneous calls to this method (in fact, as many as calls to handleEvent() or about once per row displayed).
The cache filling method performs a check to see if the currently running Job (if any) will retrieve the requested row. If so, fine (just let the Job finish). Otherwise (i.e. the user scrolled beyond what will be stored in the cache when the Job completes), kill the Job and start a new one - anyway the fetched events won't be used.
The Job fills the cache and, when it completes, it simply refreshes the table. The UI will call handleEvent() and this time the cache will be properly populated.
Bernd, could you review? Thanks. (In reply to comment #2) > Bernd, could you review? Thanks. Hi Francois Functionally, the way you addressed the deadlock issue is the right approach. However, there are a few things that needs to be look at. Here are my comments: * Possible thread synchronization problems - There are 3 threads involved: GUI-thread, the job and the thread for data request. - These threads access the members fCacheStartIndex, fCacheEndIndex and fCache of class TmfEventsTable. - The access to these members has to be thread-safe. * Before executing "fTable.getDisplay().asyncExec(new Runnable() {}" you need to check "if (!fTable.isDisposed()) {}" because the display might have been disposed in the meantime. * job.cancel() only marks the job as cancelled. It doesn't kill the job if it is in state RUNNING. The job will still run to the end. You have to implement the cancelling by checking monitor.isCanceled(). You could do this check in handleData() method of the request and if it was cancelled call just cancel() on the request. You could also check if the request was cancelled before "fTable.getDisplay().asyncExec(new Runnable() {}" and return e.g. Status.CANCEL_STATUS from the run() method in the cancel case. * There is a problem when the job is in state WAITING and the populate method is called subsequently. In this case fCacheStartIndex and fCacheEndIndex haven't been set and the job is cancelled for a subsequent call. This is a problem because this method is called once per row. Maybe it's better to set fCacheStartIndex and fCacheEndIndex before creating the job. * When using the end-button to jump to the end of the table, then the big window of the HistogramView is not updated. I need to press the end-button twice or need to click into the table. Bernd Created attachment 185256 [details]
Improved fix for the deadlock
This is an updated fix based on Bernd's comments. It turns out that there is really as possibility for a racing condition if we have a slow back-ends (like GDB Tracepoints) and a large cache.
A more serious problem occurs at refresh time: the previous patch kept sending the synchronization signal for the current event on the UI thread (again, not great) *after* the refresh job is created but *before* it completes. Therefore, a state timestamp data was used to synchronize the views.
This patch also fixes this issue by delaying the emission of the synch signal until the cache and the table rows have been updated.
Patch committed. Created attachment 185273 [details]
Optimize table refresh
The original fix lead to too many calls of method notifyUpdatedSelection() in the TmfVirtualTable. With this patch this method is called only if necessary.
Comment on attachment 185273 [details]
Optimize table refresh
Patch committed.
Comment on attachment 185273 [details]
Optimize table refresh
Patch committed.
Delivered with 0.7 |