Community
Participate
Working Groups
In some debugging situations, threads can be created and removed at a rapid rate. This can make the debug view unusable and degrade performance. A manual refresh mode should be added just for the list of threads. And a refresh button should be added to the debug view to allow refreshing the list of threads.
Created attachment 103573 [details] Patch This introduces a UpdatePolicyDecorator to customize the behavior of a base policy (automatic, manual) with special handling for stack frames in case of the debug view. The DelayedStackFrameRefreshUpdatePolicy is now a decorator for either ManualUpdatePolicy or AutomaticUpdatePolicy. It adds an "Update Policy" menu to the debug view with the choices between Automatic and Manual. and it adds a refresh button to the view toolbar. (Unfortunately, the button looks almost identical to the Restart button.)
Created attachment 103643 [details] Patch with generic handling of base update in decorator. I played around with the patch a while. I really like the approach of the update policy decorator, it's simple and it doesn't change much. But I was a little disappointed when I figured out that the decorator update was doing "instanceof" on the base update to alter its behavior. I'm hoping that instead we can use a generic mechanism sometime and allow the decorator to work without knowing the details of the base policy. So I experimented a little and came up with the following patch. Unfortunately, this patch introduces some bad behaviors: 1) The standard manual update policy works on all the elements, not just the containers. So for example when the program is terminated, and then re-launched, the launch doesn't show any children! And since the refresh action is disabled without a proper model selection, there is no way to make the launch to repaint. One way to fix this would be to use a dedicated manual update policy for threads, which would only refresh the threads list. Although, theoretically a fully manual mode is an interesting idea for the debug view and should be workable as well. The use of specialized manual mode, creates something of a problem for the decorator pattern: what if we want yet another update policy for some other elements in the view? Would we create a decorator for a decorator... I'm not sure. Maybe instead, what we need is a composite update policy which merges functionality from several other update policies. The problem with this is that implementing includes may tricky (more so than it is already). - The manual mode marking things dirty causes a lot of "..."s. This is actually true in other views, so I removed this from AbstractVMCacheProvider. The intention behind the dirty flag was to use a color to indicate which entries are stale (bug 202656), but it's still not yet implemented. For now, i think it's ok if it's just an internal flag. - Updating stack frames without updating threads leads to funky behavior. Sometimes it looks like threads have the wrong stack traces under them. One more general problem: there is no way to refresh just the children of a thread. I.e. refreshing the children forces refreshing the label. I think we need to introduce separate FLUSH flags for children and data: FLUSH_DATA, FLUSH_CHILDREN. ARCHIVE always refers to data, but DIRTY is more problematic, perhaps we'd need both DIRTY_CHILDREN and DIRTY_DATA, but only use a single UI hint to indicate either. Anyway, please take a look at the patch and let me know if you think a generic approach is workable.
(In reply to comment #2) > Created an attachment (id=103643) [details] > Patch with generic handling of base update in decorator. > > I played around with the patch a while. I really like the approach of the > update policy decorator, it's simple and it doesn't change much. But I was a > little disappointed when I figured out that the decorator update was doing > "instanceof" on the base update to alter its behavior. I'm hoping that instead > we can use a generic mechanism sometime and allow the decorator to work without > knowing the details of the base policy. So I experimented a little and came up > with the following patch. That's the dirty (pragmatic) part about it. I actually considered applying the decorator pattern also to the element tester, but avoided it for 2 reasons: - it requires the creation of new instances for each request - the DelayedStackRefreshUpdatePolicy is already a specialized update policy, only usable in the debug view, so why not restricting also the supported base policies (automatic, manual) Both arguments are not very strong, though :) > Unfortunately, this patch introduces some bad behaviors: > > 1) The standard manual update policy works on all the elements, not just the > containers. So for example when the program is terminated, and then > re-launched, the launch doesn't show any children! And since the refresh > action is disabled without a proper model selection, there is no way to make > the launch to repaint. > > One way to fix this would be to use a dedicated manual update policy for > threads, which would only refresh the threads list. I think there is not much difference to the instanceof-solution. This specialized manual update policy is then only usable together with the stack refresh update policy. > Although, theoretically a > fully manual mode is an interesting idea for the debug view and should be > workable as well. The use of specialized manual mode, creates something of a > problem for the decorator pattern: what if we want yet another update policy > for some other elements in the view? Would we create a decorator for a > decorator... I'm not sure. Maybe instead, what we need is a composite update > policy which merges functionality from several other update policies. The > problem with this is that implementing includes may tricky (more so than it is > already). I think a supergeneric solution to mix arbitrary update policies is not necessary. The set of sensible combinations should be small. In this case, we would basically need a combination of three policies: - one for the stackframes (the delayed stack refresh policy) - one for the thread list (automatic or manual) - one for the rest (automatic) To me, only 2 combinations make sense. > - The manual mode marking things dirty causes a lot of "..."s. This is > actually true in other views, so I removed this from AbstractVMCacheProvider. > The intention behind the dirty flag was to use a color to indicate which > entries are stale (bug 202656), but it's still not yet implemented. For now, i > think it's ok if it's just an internal flag. OK for me too. > - Updating stack frames without updating threads leads to funky behavior. > Sometimes it looks like threads have the wrong stack traces under them. > > One more general problem: there is no way to refresh just the children of a > thread. I.e. refreshing the children forces refreshing the label. I think we > need to introduce separate FLUSH flags for children and data: FLUSH_DATA, > FLUSH_CHILDREN. ARCHIVE always refers to data, but DIRTY is more problematic, > perhaps we'd need both DIRTY_CHILDREN and DIRTY_DATA, but only use a single UI > hint to indicate either. > Anyway, please take a look at the patch and let me know if you think a generic > approach is workable. > Hm, I don't see the point in refreshing only the children, but not their label. If you have new children, you need to update the label as well. I am basically OK with the patch, but I still think the baseTester should be used for container updates only. Everything else should be implicitly use the automatic policy.
Created attachment 103731 [details] Patch with additional tuning I reworked your patch so that in case of resumed and exited events always the automatic update policy is used. This makes the debug view update properly for those events. The baseTester is considered exclusively for container contexts to make sure that the thread list is never updated in manual mode (in automatic mode it does not make a difference). The "includes" logic of the element update tester is also slightly modified (previously non-lazy would not include lazy). I don't know if this has a major impact, bit it just looks more correct to me. Please correct if I am wrong. In the AbstractContainerVMNode I removed the handling for the FullStackRefreshEvent, as it seems to be superfluous and I had the impression that this improves the issue with losing the selection while stepping fast (bug 235268).
(In reply to comment #3) > I think a supergeneric solution to mix arbitrary update policies is not > necessary.... I guess I can buy that argument, I suppose if we later decide that we want a more sophisticated update mode, we can add it then. > Hm, I don't see the point in refreshing only the children, but not their > label. In this manual update mode for threads we want to keep the list of threads the same, but still update the state of the container to indicate that for example that it's running/stopped/terminated. (In reply to comment #4) > Created an attachment (id=103731) [details] > Patch with additional tuning > > I reworked your patch so that in case of resumed and exited events always the > automatic update policy is used. This makes the debug view update properly for > those events. I wonder if that's what we really want. In a system where many threads are both being started and terminated rapidly, refreshing all upon an exited event could be undesirable. > The baseTester is considered exclusively for container contexts to make sure > that the thread list is never updated in manual mode (in automatic mode it does > not make a difference). If the update mode is going to be specific to threads, we may want to call the menu: "threads update mode". > > The "includes" logic of the element update tester is also slightly modified > (previously non-lazy would not include lazy). I don't know if this has a major > impact, bit it just looks more correct to me. Please correct if I am wrong. I'm also somewhat confused about "includes" when it comes to these testers. Before refactoring in January, includes() was implemented to compare DMCs using DMContexts.isAncestorOf(). The purpose of it is to have the flush() method only iterate over the entries which were not flushed by one of the previous flush() calls. So the tester includes() method should return true if the given flush tester affects (flushes or marks dirty) the same or more elements than the argument tester. In case of the lazy vs. non-lazy. We want the non-lazy tester to flush all the entries that the lazy tester only marked as dirty. So I think your logic is correct that non-lazy includes lazy. > > In the AbstractContainerVMNode I removed the handling for the > FullStackRefreshEvent, as it seems to be superfluous and I had the impression > that this improves the issue with losing the selection while stepping fast (bug > 235268). > It makese sense that it improves the loosing selection when stepping fast, because the view doesn't request the list of threads anymore when stepping. There is a down-side of this in that GDB relies on the stop event refreshing the threads because its thread started/exited events are unreliable. But that's OK, it's a GDB-specific issue and GDB can override in its ContainerVMNode. But this also leaves the question: why should AbstractThreadVMNode refresh on the flush event? After all, the StackFramesVMNode adds a CONTENT flag to its parent. I actually think both container and threads should at least add a STATE flag to update their icon in case the stepping-time out event was triggered prior. This is good progress, though I think there's still some issues to resolve. I'm most concerned with wrong stack traces being returned for threads. This happens for me when I'm stepping through threads being created exited (while in manual mode). After a thread is exited, it ends up showing the stack trace of the threads that is still active. I haven't had enough time to debug this, but it seems very incorrect to me, since what should really happen is that the list of stack frames should be empty. Another thing that concerns me is that GDB's thread event reporting is somewhat limited and buggy. I think we may need to extend the PDA example to support threads to truly validate this patch.
(In reply to comment #5) > > I reworked your patch so that in case of resumed and exited events always the > > automatic update policy is used. This makes the debug view update properly for > > those events. > I wonder if that's what we really want. In a system where many threads are > both being started and terminated rapidly, refreshing all upon an exited event > could be undesirable. Agreed. I was thinking, that the exited event refers to the inferior, not a thread. > If the update mode is going to be specific to threads, we may want to call the > menu: "threads update mode". I thought that's what this bug is about ;-) > I'm most concerned with wrong stack traces being returned for threads. This > happens for me when I'm stepping through threads being created exited (while in > manual mode). After a thread is exited, it ends up showing the stack trace of > the threads that is still active. I haven't had enough time to debug this, but > it seems very incorrect to me, since what should really happen is that the list > of stack frames should be empty. I'll look into it.
(In reply to comment #6) > > I'm most concerned with wrong stack traces being returned for threads. This > > happens for me when I'm stepping through threads being created exited (while in > > manual mode). After a thread is exited, it ends up showing the stack trace of > > the threads that is still active. I haven't had enough time to debug this, but > > it seems very incorrect to me, since what should really happen is that the list > > of stack frames should be empty. > > I'll look into it. > This seems to be caused by GDB changing the active thread for some unknown reason (maybe related to spurious error messages from GDB). It is not 100% reproducible for me, but I could deduce it from the trace of mi commands.
(In reply to comment #7) Thanks Toni. This is all the more reason to not rely on GDB as the only test platform :-( On friday I started on extending PDA to support multiple execution contexts. It will take me a few days depending on how much time I can commit to it. Do you think that this feature is complete enough to apply as is right now?
(In reply to comment #8) > (In reply to comment #7) > Thanks Toni. This is all the more reason to not rely on GDB as the only test > platform :-( On friday I started on extending PDA to support multiple > execution contexts. It will take me a few days depending on how much time I > can commit to it. > > Do you think that this feature is complete enough to apply as is right now? > I'll attach an updated patch. I found some other issues, e.g. with the delta processing (VMDelta does not override getChildDelta()). I am also puzzling about bug 231436. It seems to work (locally) in automatic mode, but not in manual mode and I don't know why.
(In reply to comment #9) correction: I am also puzzling about bug 236130.
Created attachment 104179 [details] Updated patch This is as best I could get it. GDB is really quite unpredictable in its results. I tried to fix bug 236130 by limiting the delayed stack refresh to suspended events due to stepping, but this would basically invalidate the manual update policy, therefore I am leaving it as is for now. The fix to the delta processing in DefaultVMModelProxyStrategy and VMDelta avoids multiple delta childs for the same element. I am not sure this has a lot of impact, but I had the impression it improves the selection stability.
(In reply to comment #11) > This is as best I could get it... Thanks Toni, I think 236100 needs to be fixed in the run control service, so don't worry about it. I haven't had time yet to look at this patch in detail yet, but unless I see any major problems I'll go ahead and apply it. > The fix to the delta processing in DefaultVMModelProxyStrategy and VMDelta .. Great, I wondered in the past why the deltas aren't being composed like they should be, I must have just forgot the VMDelta.getChildDelta(). I'm working on extending the PDA debugger to multi-threading and if you don't mind I'd like to complete that and test this patch with it before applying it.
Created attachment 105127 [details] The "Updated patch" merged after the changes from bug 236765. Hi Toni, At last I completed the PDA multi-threading bug and I think we can test this patch more fairly. The only issue I found so far is that after a thread exits, all the threads end up with labels of "...". Could you re-test your patch with PDA and see what else you find. You can use the vmtest9.pda test program.
Created attachment 105152 [details] Final patch (+committed) The previous patch did not contain all changes (E.g. VMDelta). I added them back in and did some minor tweaks after testing with the new PDA. In manual mode, stale exited threads are displayed as "<unavailable>" now instead of "...".
Created attachment 105210 [details] Final final patch. This is the patch I actually committed although I marked the other one for IP log purposes.
(In reply to comment #14) > Created an attachment (id=105152) [details] > Final patch > > The previous patch did not contain all changes (E.g. VMDelta). I added them > back in and did some minor tweaks after testing with the new PDA. > In manual mode, stale exited threads are displayed as "<unavailable>" now > instead of "...". > Thanks Toni for fixing the patch. I spent quite a bit of time debugging this feature and I think it's at a good point, although there are still a few improvements it can use, but those will have to wait for other features in the cache. (I'll file separate bugs) The main thing I realized is that the container and threads nodes were not (and still for the most part are not) using the cache for label data. This is why even though the container is in manual refresh mode, when I suspend the container, it's label still updates. This is good, but it will not work after we stop using the deprecated getModelData() and start using property-based label provider. Anyway, other minor things I found: 1) I added a separate ThreadsUpdateTester for updating elements on events other than suspended and full-stack-refresh. I think this is the more correct behavior. 2) IModelDelta.CONTENT implies IModelDelta.STATE, so we don't need to OR those flags. 3) IModelDelta.ARCHIVE flag is not needed in Debug view since changed value high-lighting is not used. I know that standard automatic and manual update modes include them, but it's probably not necessary to duplicate them just for this purpose. Maybe we could use a way to extend the testers themselves so we could re-use the testers from those update policies better.
That's strange. I committed the change, but forgot to mark it as fixed. Randy please review if you have time. Since Toni is almost a committer, you can assign it to him for review if you prefer.
I looked at the code pretty carefully. Most of it seemed correct, although I did not totally check every line. Randy
DD 1.1 reelased!
