Bugzilla – Full Text Bug Listing
| Summary: | Make debugging proxy resolutions easier | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Ed Willink <ed> | ||||||
| Component: | Core | Assignee: | Ed Merks <Ed.Merks> | ||||||
| Status: | RESOLVED WONTFIX | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | Kenn.Hussey | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows Vista | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Ed Willink
Resolve is normally not called for case 1, except directly by the client; the object isn't a proxy. So that leaves only two interesting cases, 2 and 3. Case 3 is the final return statement and case 2 is the other return statement. One can set breakpoints on catches too. I don't see how any changes will make life easier. I've investigated why I'm seeing so many EcoreUtil::resolves of non-proxies. It's the internal EcoreUtil::resolve recursion after successful resolution of a proxy, which results in resolvedObject != null (with a null proxy) and resolvedObject != proxy. Perhaps the recursive call should be guarded by a proxyURI!=null, to satisfy your statement that EcoreUtil::resolve is never called with a null proxyURI by client code. In practice the recursion will very rarely be needed so the extra guard will improve performance. That's not a bad idea. Can you check that it does improve performance? I imagine it generally wouldn't hurt because proxies don't generally chain... Created attachment 169609 [details]
Patch to guard recursion
Attached patch guards the recursion, and ensures that a successful result uses a distinct return simplifying debugging.
Some JUnit tests run very slightly faster (43.080 rather than 43.110 seconds), but not by enough to be sure that it is faster, but that is to be expected since EcoreUtil.resolve is only a small part of the tests.
By inspection, any non-chained resolution must win since the same number of statements are executed, but one less call.
As I look at the change, the redundancy is still bothersome... Can it really be argued that a conditional breakpoint is not able to distinguish all the interesting cases? The original comment was a request to assist debugging. Further investigation identified the guaranteed redundant recursion. The patch to prevent the recursion also facilitated debugging. There is a halfway house avoiding the redundant recursion without aiding debugging, which may save a couple of bytes of code, at the expense of requiring more effort to set up a breakpoint. Created attachment 178275 [details]
Patches to factor out the recursion
The problem to my mind is that the code to guard against recursion is repeated when recursion is necessary. I.e., we go from redundant recursion (not really redundant because there is a check that must be done to determine redundancy) to having a redundant check done by redundant code that's checks a condition that's redundantly checked a second time in the case of recursion isn't redundant. So while we go from something that's arguably not redundant we end up with something that most definitely has redundancy. And we should do that because it makes setting a breakpoint easier. I'm not convinced that's a good trade-off.
At least this patch avoids calling eProxyURI redundantly, though at the cost of more complex code...
Just used this for a real problem. Your comments in #1 are now true, so debugging is easy and inserting a debug retry is pretty easy too. Conditional breakpoints should be sufficient. I'm confused. You've WONTFIXed after providing a patch that is required to make your statements in #1 true. I confirmed that this patch is useful, in particular, in accord with the intervening discussion it avoids the redundant recursion that made breakpoints very hard; I don't even know how to do it. Statement 1 was made without the patch I've attached so while the relation between your comment and the patch might be clear to you, it's obviously not so clear to me. As I said, I'm averse to making the logic more complex just to appease debugging, which I'm sure can be accomplished with conditional breakpoints. It's not as if I've never needed to debug what's going on with proxy resolution myself... I find a breakpoint on a proxy resolution failure is very useful since it immediately focusses on the URI/model registrations rather than the downstream code that malfunctions as consequence. Xtext has very elaborate URIs for lazy resolution. These can fail in interesting ways. I sometimes edit EcoreUtil.resolve to add a retry so that I can single step the retry. --- The original report was a request for debug help. In comment #1 you claimed the problem was soluble with conditional breakpoints. In comment #2 I identified an unintended recursion that undermined your comment #1. In comment #4 I provided a patch to fix the recursion. In comment #7 you reworked the patch. In comment #8 I welcomed the reworked patch. In comment #9 you WONTFIXed the patch, perhaps considering only the original report and #1 and overlooking the recursion problem identified in #2. It is important to be able to easily detect proxy resolution failures. Please apply the patch, which fixes the recursion and consequently facilitates debugging. I'll ask for Kenn's unbiased opinion. BTW, wouldn't the debugger's ability to "drop to frame" be sufficient for retrying the logic in the case that resolution fails? In fact, looking at the original code, wouldn't a conditional breakpoint on the final return checking for proxy.eIsProxy() being true be sufficient to catching all failure cases? At that point, dropping to the start of the frame would allow one to trace the cause of the failure. This seems very easy, but perhaps I miss something... (In reply to comment #13) > I'll ask for Kenn's unbiased opinion. I don't see the need to make the code more complex just to aid in debugging, but removal of the redundant path that occurs during recursion should probably be done... Kenn, Note that there is no redundant path. After resolving a proxy, we must check that the resolved result isn't itself a proxy. That test is done in the recursive call and is required. Adding code to test it again would be redundant. I'll make this assertion: setting a conditional breakpoint on the final return that checks for eIsProxy is true will find all cases of failure. Dropping the stack frame and retrying will help determine why it failed. Of course my assertion might be wrong, but right now I don't see that it's wrong. The test that a resolved proxy is a proxy is indeed not redundant, but the recursion to do that test is. Testing prior to recursion avoids the recursion for the almost ubiquitous use case of a resolution success. Ed M correctly pointed out in comment #1 that the debugging problem doesn't exist when there is no recursion. Unfortunately there is. My patch added a 3 line guard against the recursion; it did not add code to facilitate debugging since removal of the recursion solves the debugging problem. For the normal success case of resolution of a one long chain, my patch saves one function call, since the guard test in the redundant recusion is executed executed outside to guard the recursion. For the normal failure case of a non-resolution there is no change. For the, I think, unusual case of a resolution of an N-long chain, my patch saves one function call but adds N-1 redundant extra guard tests. So it probably costs slightly for chains of 3 and above. Ed M's patch makes the control flow more elegant, but by factoring out two alternate behaviours as new functions, the patch adds more code and an extra function call on the normal cases. The unusual case does not incur the redundant guard tests. I don't think the extra code or extra function call on the normal case justifies the benefit for the unususal case. I think that my patch is more pragmatic. Please apply. You won't convince me to add redundant logic. I have an aversion to that. You're not doing a good job convincing me it isn't already easy to debug the case of failure. That's the only point of this bugzilla. We do all have bigger fish to fry so let's avoid going down already-visited paths. (In reply to comment #15) > (In reply to comment #13) > > I'll ask for Kenn's unbiased opinion. > > I don't see the need to make the code more complex just to aid in debugging, > but removal of the redundant path that occurs during recursion should probably > be done... In #17, I have provided a cost analysis of the two patches that solve the recursion path. Is there a third way? Otherwise please proceed with one of the two. |