Bugzilla – Full Text Bug Listing
| Summary: | [variables] Add "hasSubExpressions" method to IExpressions | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Nobody - feel free to take it <nobody> | ||||||||||||||
| Component: | cdt-debug-dsf | Assignee: | Nobody - feel free to take it <nobody> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | marc.khouzam, mario.pierro, pawel.1.piech | ||||||||||||||
| Version: | 7.0 | Flags: | marc.khouzam:
review+
pawel.1.piech: review+ nobody: review+ |
||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows 7 | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | 317324, 317325 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Nobody - feel free to take it
You may also be able to implement has children just by looking at the element type. Having the model return a false positive for has-children is not a big deal because the view handles it pretty gracefully. (In reply to comment #1) > You may also be able to implement has children just by looking at the element > type. Having the model return a false positive for has-children is not a big > deal because the view handles it pretty gracefully. Yes, you're right. I have been busy implementing the IRegisters service based on variable objects that's why I'm so focused on it. Created attachment 171126 [details]
Proposed solution.
This patch adds a new interface that extends IExpressions2 and modifies the exisiting implementation to take advantage of the new API.
Marc, please review this patch. It introduces a new interface and I don't know if it can be accepted for the maintenance branch. 1- the IExpressions3 should be officially approved by Pawel, as it is in DSF 2- Can you remove the implements of IExpressions2 from MIExpressions 3- Why the change to MIVariableManager? Assuming #3 is a mistake, then I don't think this is as efficient as what you would like. As proposed (without the change to MIVariableManager), I believe the call to hasSubExpressions() will cause as many -var-list-children as getSubExpressionCount(). I was thinking that we have two options: a- we change ExpressionDMData to no longer know the number of children, but only hasChildren(). No one actually uses ExpressionDMData.getNumChildren since it is not part of IExpressionDMData. I don't know why we have that method. This means also changing ExprMetaGetVarInfo to not know the number of children, but only hasChildren(). This will avoid all attempts to list the children for IExpressions.getExpressionData() and the new IExpressions.hasSubExpressions(). Of course, the problem is that is a non-backwards-compatible change. We could actually keep this change backwards-compatible, but ExpressionDMData.getNumChildren() would not work and we should indicate that clearly. b- We could not reuse ExprMetaGetVar but define a new ExprMetaGetHasChildren, which would simply check if there are children or not. This can be done without using -var-list-children (In reply to comment #5) Thanks Marc. > 2- Can you remove the implements of IExpressions2 from MIExpressions Sure. > 3- Why the change to MIVariableManager? > > Assuming #3 is a mistake, then I don't think this is as efficient as what you > would like. As proposed (without the change to MIVariableManager), I believe > the call to hasSubExpressions() will cause as many -var-list-children as > getSubExpressionCount(). > Not sure I understand you. The changes in MIVariableManager are essential. You are absolutely right: without it hasSubExpressions() will cause as many -var-list-children as getSubExpressionCount(). The differences between two calls are following: - hasSubExpression() always uses the "numchild" value returned by "var-create", so no "var-list-children" calls - getSubExpressionsCount() depends on isNumChildrenHintTrustworthy() to decide whether to make "var-list-children" call. This problem is specific to variable objects that's why I changed MIVariableManager. > I was thinking that we have two options: > > a- we change ExpressionDMData to no longer know the number of children, but > only hasChildren(). No one actually uses ExpressionDMData.getNumChildren since > it is not part of IExpressionDMData. I don't know why we have that method. Yes, it is confusing. I thought I had to use a higher level classes. > This means also changing ExprMetaGetVarInfo to not know the number of children, > but only hasChildren(). This will avoid all attempts to list the children for > IExpressions.getExpressionData() and the new IExpressions.hasSubExpressions(). > > Of course, the problem is that is a non-backwards-compatible change. We could > actually keep this change backwards-compatible, but > ExpressionDMData.getNumChildren() would not work and we should indicate that > clearly. > As far as I understand ExprMetaGetVarInfo is associated with the data returned by var-create. If we go with this option I think that we should keep getNumChildren(). The way GDB manages the members of C++ classes should be modified instead. It should be possible to switch off "public/protected/private" children in the var-create command. > b- We could not reuse ExprMetaGetVar but define a new ExprMetaGetHasChildren, > which would simply check if there are children or not. This can be done > without using -var-list-children I'll take a look at this option but first I would like to clarify the above issues. (In reply to comment #5) > 1- the IExpressions3 should be officially approved by Pawel, as it is in DSF After reading Marc's comments it seems to me that it may be more appropriate to add a IExpressionDMData.hasChildren(), because it would reduce the need for another async call by the client. In the javadoc indicate that this method could give false positives. I.e. it may be more efficient for the debugger to make a best guess at whether an expression has children, and to find out for sure clients should call getSubExpressionCount. I would prefer not to add a new interface in the DSF maintenance release (@since 2.1.1?). Maybe you could work around it by accessing an internal method in DSF-GDB UI plugin. > Of course, the problem is that is a non-backwards-compatible change. We could > actually keep this change backwards-compatible, but > ExpressionDMData.getNumChildren() would not work and we should indicate that > clearly. Could you use getNumChildren() to return the "numchild value" (In reply to comment #7) > > Of course, the problem is that is a non-backwards-compatible change. We could > > actually keep this change backwards-compatible, but > > ExpressionDMData.getNumChildren() would not work and we should indicate that > > clearly. > Could you use getNumChildren() to return the "numchild value" I don't understand... I thought you meant that when getNumChildren() is called (if ever) it would make a call to the service and actually fetch the children only then. I think that would work. Is that what you meant? (In reply to comment #6) > Not sure I understand you. The changes in MIVariableManager are essential. You > are absolutely right: without it hasSubExpressions() will cause as many > -var-list-children as getSubExpressionCount(). > The differences between two calls are following: > > - hasSubExpression() always uses the "numchild" value returned by "var-create", > so no "var-list-children" calls > > - getSubExpressionsCount() depends on isNumChildrenHintTrustworthy() to decide > whether to make "var-list-children" call. > > This problem is specific to variable objects that's why I changed > MIVariableManager. All true. The problem is that ExprMetaGetVar is also used by IExpressions.getExpressionData() so the change to MIVariableManager will affect that call and will make the returned IExpressionDMData not have the right number of children. Of course, the number of children is not actually used, so I think you are effectively implementing suggestion (a), but not completely since it will break ExpressionDMData.getNumChildren() > As far as I understand ExprMetaGetVarInfo is associated with the data returned > by var-create. If we go with this option I think that we should keep > getNumChildren(). The way GDB manages the members of C++ classes should be > modified instead. It should be possible to switch off > "public/protected/private" children in the var-create command. That would be much nicer. But we'll have to keep dealing with older GDBs for a while, so I think we still need a proper handling in CDT. Replacing ExpressionDMData.getNumChildren with ExpressionDMData.hasChildren is sufficient I think. (In reply to comment #7) > (In reply to comment #5) > > 1- the IExpressions3 should be officially approved by Pawel, as it is in DSF > > After reading Marc's comments it seems to me that it may be more appropriate to > add a IExpressionDMData.hasChildren(), because it would reduce the need for > another async call by the client. In the javadoc indicate that this method > could give false positives. I.e. it may be more efficient for the debugger to > make a best guess at whether an expression has children, and to find out for > sure clients should call getSubExpressionCount. > I would prefer not to add a new interface in the DSF maintenance release > (@since 2.1.1?). Maybe you could work around it by accessing an internal > method in DSF-GDB UI plugin. BTW, I like this idea. But why the false positives? For GDB at least, we can always know if there are children or not (we can't know the exact number without -var-list-children sometimes, but we can know != 0) (In reply to comment #10) > > I.e. it may be more efficient for the debugger to > > make a best guess at whether an expression has children, and to find out for > > sure clients should call getSubExpressionCount. > > BTW, I like this idea. But why the false positives? For GDB at least, we can > always know if there are children or not (we can't know the exact number > without -var-list-children sometimes, but we can know != 0) Sorry, I get it now. Some debuggers may choose to avoid properly answering such a IExpressions.hasChildren() for efficiency but to always return true instead. (In reply to comment #7) > (In reply to comment #5) > > 1- the IExpressions3 should be officially approved by Pawel, as it is in DSF > > After reading Marc's comments it seems to me that it may be more appropriate to > add a IExpressionDMData.hasChildren(), because it would reduce the need for > another async call by the client. In the javadoc indicate that this method > could give false positives. I.e. it may be more efficient for the debugger to > make a best guess at whether an expression has children, and to find out for > sure clients should call getSubExpressionCount. > I would prefer not to add a new interface in the DSF maintenance release > (@since 2.1.1?). Maybe you could work around it by accessing an internal > method in DSF-GDB UI plugin. I don't think we need to mention it. False positives are used in many situations. > Could you use getNumChildren() to return the "numchild value" That's what I am proposing in this patch. (In reply to comment #9) > I think you are effectively implementing suggestion (a), but not completely > since it will break ExpressionDMData.getNumChildren() My point is that ExpressionDMData.getNumChildren() is an MI extension, so it should return the "numchild" value. See comment 7. Anyway, I'll try to implement it without API changes but we should consider to make the appropriate API changes later. (In reply to comment #13) > (In reply to comment #9) > > I think you are effectively implementing suggestion (a), but not completely > > since it will break ExpressionDMData.getNumChildren() > > My point is that ExpressionDMData.getNumChildren() is an MI extension, so it > should return the "numchild" value. See comment 7. I see what you mean now. When you guys say "numchild" you mean what GDB returns in -var-create. The iffy thing here is that when GDB says "numchild" is 2 for private/public, then -var-list-children will actually create two children: private and public. But in our case, we ignore that level of children, so when we will say that there are 2 children based on "numchild", a call to getSubExpressions() could return something different. This seems bad to me. But again ExpressionDMData.getNumChildren() is not even used so it feels like a lot of energy spend to support something we don't use :-) Maybe we can simply javadoc that method to say that it should only be used to know if children exists and not as an exact number, getSubExpressionsCount should be used instead for an exact number. Basically, making it behave like hasChildren() without renaming it to avoid breaking the API (In reply to comment #15) > But again ExpressionDMData.getNumChildren() is not even used so it feels like a > lot of energy spend to support something we don't use :-) Maybe we can simply > javadoc that method to say that it should only be used to know if children > exists and not as an exact number, getSubExpressionsCount should be used > instead for an exact number. Basically, making it behave like hasChildren() > without renaming it to avoid breaking the API Sounds good to me. We can deprecate getNumChildren() to avoid confusion. But that's an API change. (In reply to comment #7) > I would prefer not to add a new interface in the DSF maintenance release > (@since 2.1.1?). Maybe you could work around it by accessing an internal > method in DSF-GDB UI plugin. I can add a new internal DSF/GDB service (say GdbMIExpressions) that extends MIExpressions and implements hasSubExpressions() and overwrite updateHasElementsInSessionThread() in GdbVariableVMNode. The only problem is there is no package for internal services in DSF/GDB. Is it OK to add an internal package? (In reply to comment #17) > (In reply to comment #7) > > I would prefer not to add a new interface in the DSF maintenance release > > (@since 2.1.1?). Maybe you could work around it by accessing an internal > > method in DSF-GDB UI plugin. > > I can add a new internal DSF/GDB service (say GdbMIExpressions) that extends > MIExpressions and implements hasSubExpressions() and overwrite > updateHasElementsInSessionThread() in GdbVariableVMNode. > The only problem is there is no package for internal services in DSF/GDB. Is it > OK to add an internal package? I don't know if that is ok for a maintenance branch, but I don't see why it wouldn't be. Besides that point, I'm fine with that. I'm a little confused about what you will do in the end though :-) I guess I'll wait for patch. (In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #7) > > > I would prefer not to add a new interface in the DSF maintenance release > > > (@since 2.1.1?). Maybe you could work around it by accessing an internal > > > method in DSF-GDB UI plugin. > > > > I can add a new internal DSF/GDB service (say GdbMIExpressions) that extends > > MIExpressions and implements hasSubExpressions() and overwrite > > updateHasElementsInSessionThread() in GdbVariableVMNode. > > The only problem is there is no package for internal services in DSF/GDB. Is it > > OK to add an internal package? > > I don't know if that is ok for a maintenance branch, but I don't see why it > wouldn't be. Besides that point, I'm fine with that. > > I'm a little confused about what you will do in the end though :-) > I guess I'll wait for patch. I thought (wrongly) that I could extend MIExpressions but I need access to some of its private members :( As there are other issues that need to be addressed to improve performance let's wait with this patch for an appropriate realease. Created attachment 171357 [details]
Updated patch.
The important part of the original patch is missing, no wander Marc was confused. The updated patch still contains a new interface, so it can only be applied to a major version. (In reply to comment #21) > The important part of the original patch is missing, no wander Marc was > confused. The updated patch still contains a new interface, so it can only be > applied to a major version. Instead of IExpressions3.hasSubExpressions(), could we add an IExpressionDMData3.hasSubExpressions()? Can you see any reason that would be a bad idea? (In reply to comment #12) > I don't think we need to mention it. False positives are used in many > situations. They may be used in many situations, but it's an important fact about the API :-) (In reply to comment #22) > Instead of IExpressions3.hasSubExpressions(), could we add an > IExpressionDMData3.hasSubExpressions()? Can you see any reason that would be a > bad idea? I can't come up with any reason against it. It just seems to me that "hasSubExpressions" and "getSubExpressionCount" should belong to the same interface. IExpressionDMData is probably the better place because it contains the type information which (as you suggested earlier) can be used to implement these two methods. (In reply to comment #23) > I can't come up with any reason against it. It just seems to me that > "hasSubExpressions" and "getSubExpressionCount" should belong to the same > interface. IExpressionDMData is probably the better place because it contains > the type information which (as you suggested earlier) can be used to implement > these two methods. It's just a matter of chunking information in such a way that it is convenient for the client, but not so much so as to degrade performance. I'm guessing that in the worst case, the service can calculate the has-children property based on expression type, which is already in IExpressionDMData, so as not to force the service to retrieve a much larger chunk of data. A separate hasSubExpressions() call seems like an overkill to me... Created attachment 171436 [details]
Patch based on the extension of IExpressionDMData interface
According to Pawel's request this patch uses an extension of IExpressionDMData interface.
(In reply to comment #22) > Instead of IExpressions3.hasSubExpressions(), could we add an > IExpressionDMData3.hasSubExpressions()? Can you see any reason that would be a > bad idea? See https://bugs.eclipse.org/bugs/attachment.cgi?id=171436. The disadvantage is that we have to get data to check whether it is an instance of the new interface. If it is not we have to make another async call from the previos call to get the children count. (In reply to comment #26) > See https://bugs.eclipse.org/bugs/attachment.cgi?id=171436. The disadvantage is > that we have to get data to check whether it is an instance of the new > interface. If it is not we have to make another async call from the previos > call to get the children count. Oh wow, you're right, that is ugly. In that case, what do you think of adding: interface IExpressions3 extends IExpressions { public interface IExpressionDMDataExtension extends IExpressionDMData { boolean hasChildren(); } void getExpressionDataExtension( IExpressionDMContext dmc, DataRequestMonitor<IExpressionDMDataExtension> rm); } The only thing I find annoying is the lack of symmetry between IExpression3 and IExpressionDMDataExtension names. (In reply to comment #27) > Oh wow, you're right, that is ugly. In that case, what do you think of adding: > > interface IExpressions3 extends IExpressions { > public interface IExpressionDMDataExtension extends IExpressionDMData { > > boolean hasChildren(); > } > > > void getExpressionDataExtension( > IExpressionDMContext dmc, > DataRequestMonitor<IExpressionDMDataExtension> rm); > } > > The only thing I find annoying is the lack of symmetry between IExpression3 and > IExpressionDMDataExtension names. Yes, this will work and I too don't like the names. I am afraid this would follow by IExpressions<N> and IExpressionDMDataExtension<N> :( From going through this bug again, I believe the ball is in Mikhail's court now. If I'm wrong and I'm supposed to do something, just let me know :-) (In reply to comment #29) > From going through this bug again, I believe the ball is in Mikhail's court > now. > > If I'm wrong and I'm supposed to do something, just let me know :-) Your right, it's in my court now :) Created attachment 172169 [details]
Patch based on Pawel's proposal
Now ball is in Marc's or Pawel's court:) Created attachment 172242 [details]
Added missing since tag.
This patch extends the existing API so the upversioning of dsf and dsf.gdb plug-ins is required. It is assumed the next version of dsf plug-ins is 2.2 and 3.1 for dsf.gdb plug-ins.
The interface extensions are OK by me. Thanks Pawel. Applied to HEAD. *** cdt cvs genie on behalf of mkhodjai *** Bug 315677 - [variables] Add "hasSubExpressions" method to IExpressions [+] IExpressions3.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/debug/service/IExpressions3.java?root=Tools_Project&revision=1.1&view=markup [*] MIVariableManager.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIVariableManager.java?root=Tools_Project&r1=1.12&r2=1.13 [*] MIExpressions.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIExpressions.java?root=Tools_Project&r1=1.18&r2=1.19 [*] VariableVMNode.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/viewmodel/variable/VariableVMNode.java?root=Tools_Project&r1=1.20&r2=1.21 Created attachment 172772 [details]
Deprecate method and fix JUnit tests
I've deprecated the old getNumChildren() method as we talked about.
I've also fixed the JUnit tests to work with the changes from this bug.
Committed to HEAD.
Mikhail's changes look good to me. Mikhail, can you review the last patch I committed to make sure it is in line with what you had in mind. Also, let me know if you have any issues with getting the JUnit tests to run on your machine :-) (In reply to comment #39) > Also, let me know if you have any issues with getting the JUnit tests to run on > your machine :-) Marc, please excuse my ignorance, but what exactly I need to install to get org.eclipse.test.performance plug-in? I really don't want to install the entire TPTP in my workspace - have had enough troubles with it. That's the main reason I didn't install the dsf tests. *** cdt cvs genie on behalf of mkhouzam *** Bug 315677: The getNumChildren() method was not actually used and was causing some inefficiency, so we deprecate it in favor of hasChildren() and getSubExpressionCount(). Also fix the JUnit tests accordingly. [*] MIVariableManager.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIVariableManager.java?root=Tools_Project&r1=1.13&r2=1.14 [*] MIExpressions.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIExpressions.java?root=Tools_Project&r1=1.19&r2=1.20 [*] ClassAccessor.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/ClassAccessor.java?root=Tools_Project&r1=1.5&r2=1.6 [*] MIExpressionsTest.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/MIExpressionsTest.java?root=Tools_Project&r1=1.10&r2=1.11 (In reply to comment #40) org.eclipse.test.performance is part of Eclipse core project, it has no outside dependencies. You can get it from /cvsroot/eclipse. (In reply to comment #40) > (In reply to comment #39) > > Also, let me know if you have any issues with getting the JUnit tests to run on > > your machine :-) > Marc, please excuse my ignorance, but what exactly I need to install to get > org.eclipse.test.performance plug-in? I really don't want to install the entire > TPTP in my workspace - have had enough troubles with it. That's the main reason > I didn't install the dsf tests. The DSF-GDB JUnit tests are under org.eclipse.cdt.tests.dsf.gdb which you should check-out in your workspace. You can then refer to http://wiki.eclipse.org/CDT/cdt-debug-dsf-gdb#Automated_testing on how to run them. I run them on Linux and John is running them on Windows (In reply to comment #39) > Mikhail, can you review the last patch I committed to make sure it is in line > with what you had in mind. The patch looks good to me. |