Bugzilla – Full Text Bug Listing
| Summary: | [tracepoint] Cache "trace-status" command | ||
|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Nobody - feel free to take it <nobody> |
| Component: | cdt-debug-dsf-gdb | Assignee: | Project Inbox <cdt-debug-dsf-gdb-inbox> |
| Status: | NEW --- | QA Contact: | Jonah Graham <jonah> |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | cdtdoug, pawel.1.piech |
| Version: | 8.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux-GTK | ||
| Whiteboard: | |||
|
Description
Nobody - feel free to take it
Yeah, it is really annoying to see '-trace-status' being called so many times. I wonder how much caching we can do though. (gdb) interpreter-exec mi "-trace-status" ^done,supported="1",running="0",frames="0",frames-created="0",buffer-size="5242880",buffer-free="5242880",disconnected="0",circular="0" -trace-status reports information that can change at any time: 1- the amount of data that has been collected 2- if the trace experiment is running or not. A trace experiment can stop on its own if the buffer gets full. And we get no MI event for that (I suggested having one, but it didn't happen (yet?)) So, caching that command might not be possible... (In reply to comment #1) > -trace-status reports information that can change at any time: > 1- the amount of data that has been collected > 2- if the trace experiment is running or not. A trace experiment can stop on > its own if the buffer gets full. And we get no MI event for that (I suggested > having one, but it didn't happen (yet?)) > Not sure I understand it. Is this because of the non-stop mode? (In reply to comment #2) > Not sure I understand it. Is this because of the non-stop mode? I hadn't realized it, but you are right, it only is a problem for non-stop. (Well, actually, it would also apply with all-stop and target-async, but since we don't use that mode in CDT, that point is moot) So, when you start tracing and then resume your execution in non-stop, you can keep sending -trace-status commands, which will return different results as the tracing data is being collected. In all-stop, since GDB is not responding, there is no point in sending multiple -trace-status, since they will all happen at once, when the execution stops. So, I guess we could cache in the case of all-stop, if you think it is worth it. (In reply to comment #3) > I hadn't realized it, but you are right, it only is a problem for non-stop. > (Well, actually, it would also apply with all-stop and target-async, but since > we don't use that mode in CDT, that point is moot) > > So, when you start tracing and then resume your execution in non-stop, you can > keep sending -trace-status commands, which will return different results as the > tracing data is being collected. > > In all-stop, since GDB is not responding, there is no point in sending multiple > -trace-status, since they will all happen at once, when the execution stops. > > So, I guess we could cache in the case of all-stop, if you think it is worth > it. As I understand we need at least three MI events: 1. Trace experiment started (this one would be useful to support GDB console) 2. Trace experiment stopped 3. Trace status changed Do you think you can convince the GDB community of adding these events? (In reply to comment #4) > As I understand we need at least three MI events: > 1. Trace experiment started (this one would be useful to support GDB console) > 2. Trace experiment stopped The above two sound good. > 3. Trace status changed This one can flood us if a lot of trace data is collected. In the TraceControl view, I added a refresh button to allow the user to poll the trace status, specifically because we cannot have a pushed event to us. > Do you think you can convince the GDB community of adding these events? There is some work planned for having a fully functional GDB console, through the help of GDB events. This is planned for this year or next. There should be some event caused by a 'tstop' command. We'll just have to see if that event will also be sent when the trace experiment stops on its own. If it does not, it should be easy to add it, once the event has been included in GDB's MI events. This 'problem' is annoying me again :) I suggest we cache the -trace-status command for 100ms and then clear the cache. This will avoid multiple -trace-status commands being sent one immediately after the other, and yet will allow us to refresh the status periodically. I don't think 100ms of 'stale' data will be a problem. Small fix pushed to Gerrit: https://git.eclipse.org/r/19876 The idea is to use a commandCache just for -trace-status, and clear that cache after 300ms. Through testing, I found that 100ms wasn't enough as I would see too many -trace-status, but 300ms was better without affecting the user. Mikhail, what do you think? Just to be clear, this is only a partial solution. As Mikhail pointed out, when we have an MI event for 'tstop' and 'tstart', we could have a smarter caching. For example, when no experiment is running, there would be no need to refresh '-trace-status' since nothing would be changing. The only MI Events still missing in GDB are for 'tstop' and 'tstart'. The latest attempt at getting those in has started 5 days ago: https://sourceware.org/ml/gdb-patches/2013-12/msg00495.html Once those two events are accepted in GDB, we can fix this bug more completely. (In reply to Marc Khouzam from comment #7) > Small fix pushed to Gerrit: > https://git.eclipse.org/r/19876 > > The idea is to use a commandCache just for -trace-status, and clear that > cache after 300ms. Through testing, I found that 100ms wasn't enough as I > would see too many -trace-status, but 300ms was better without affecting the > user. > > Mikhail, what do you think? As far as I understand, the trace status is used to update the actions of the Trace Control view when the session is suspended. Is it possible that we would end up with these actions being wrongly enabled/disabled because of the stale data in the cache? (In reply to Mikhail Khodjaiants from comment #9) > As far as I understand, the trace status is used to update the actions of > the Trace Control view when the session is suspended. Is it possible that we > would end up with these actions being wrongly enabled/disabled because of > the stale data in the cache? That is a good question that I hadn't thought of. Looking at how -trace-status is being called, it is being called twice in quick succession by two actions: GdbSelectNextTraceRecordCommand and GdbSelectPrevTraceRecordCommand. The result of -trace-status is then used to know if there is at least one trace record collected to know if each action could be enabled, but only if there is no on-going trace experiment. The risk with this patch would be that a trace record could be collected during the 300ms caching and therefore not be reflected by the buttons. However, even if this happens it will not have an impact because the only time a trace record can be collected by GDB is when a trace experiment is on-going, in which case, the two above actions will remain disabled. So I think it is safe. But if you feel this make is brittle for future changes, we can not use the patch. The only benefit is cutting in half the -trace-status sent because the two above actions send -trace-status in quick succession every time they need to test their enablement. (In reply to Marc Khouzam from comment #10) > (In reply to Mikhail Khodjaiants from comment #9) > > > As far as I understand, the trace status is used to update the actions of > > the Trace Control view when the session is suspended. Is it possible that we > > would end up with these actions being wrongly enabled/disabled because of > > the stale data in the cache? > > That is a good question that I hadn't thought of. > > Looking at how -trace-status is being called, it is being called twice in > quick succession by two actions: GdbSelectNextTraceRecordCommand and > GdbSelectPrevTraceRecordCommand. The result of -trace-status is then used > to know if there is at least one trace record collected to know if each > action could be enabled, but only if there is no on-going trace experiment. > > The risk with this patch would be that a trace record could be collected > during the 300ms caching and therefore not be reflected by the buttons. > However, even if this happens it will not have an impact because the only > time a trace record can be collected by GDB is when a trace experiment is > on-going, in which case, the two above actions will remain disabled. So I > think it is safe. > > But if you feel this make is brittle for future changes, we can not use the > patch. The only benefit is cutting in half the -trace-status sent because > the two above actions send -trace-status in quick succession every time they > need to test their enablement. If you think it is safe, I am OK with the patch. Maybe we should add a comment for the future changes. (In reply to Mikhail Khodjaiants from comment #11) > If you think it is safe, I am OK with the patch. Maybe we should add a > comment for the future changes. Thanks Mikhail. Done and committed to master. I will leave the bug open to improve the cache once we have the proper GDB events. |