Bugzilla – Full Text Bug Listing
| Summary: | Extended properties on nested sequences are silently discarded | ||
|---|---|---|---|
| Product: | [Modeling] Epsilon | Reporter: | Antonio Garcia-Dominguez <agarcdomi> |
| Component: | Core | Assignee: | Dimitris Kolovos <dkolovos> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 172555 [details]
Patch for reusing the wrappers in EolTypeWrapper
Created attachment 172556 [details]
Patch for avoiding unwrapping as much as possible in PointExecutor
Created attachment 172557 [details]
Sample EOL script which illustrates the problem
Thanks for reporting this. Please see comments below: > * first try calling with wrapped values, and then try unwrapped values: > this fixes this problem, does not lose any of the available > API, and avoids the useless unwrapping and rewrapping that gets done > here. But I might be missing something here: is there a reason why > unwrapped values should be tried first, like now? I'm sure I've changed the order numerous times in the past and eventually came to the conclusion that this was the best trade-off (though I don't remember exactly why). > * change EolTypeWrapper so that when wrapping an object that was wrapped > before, the original wrapper will be returned. This uses more memory, > but it might solve the problem in a more general way. I've also tried this but the memory footprint gets bigger and bigger as you're using Epsilon and eventually requires you to restart Eclipse :S Object wrapping has always been a major source of trouble and I believe it makes sense to try to come up with a solution that does away with it altogether. I'm inclined not to fix this at the moment - assuming that's not a show-stopper for you - and address it in the context of a wider refactoring. Any thoughts on this? (In reply to comment #4) > I'm sure I've changed the order numerous times in the past and eventually came > to the conclusion that this was the best trade-off (though I don't remember > exactly why). That's what I thought as well: I guess there just isn't a definitive answer in this case. We can drop that patch, I guess. > I've also tried this but the memory footprint gets bigger and bigger as you're > using Epsilon and eventually requires you to restart Eclipse :S Ohh, I see. Adding it to the map means adding a strong reference to it, so we'll never garbage collect that object even if Epsilon does not need it anymore :-/. We could use weak references to solve the problem, though. There is a WeakHashMap in the JDK which uses weak references and automatically garbage collects keys for which no strong references exist, so it's perfect for what we want. Here: http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html It implements the HashMap interface, so we only have to change a line in the actual code. I'll update the patch for the second approach with this, so you can try it out :-). > Object wrapping has always been a major source of trouble and I believe it > makes sense to try to come up with a solution that does away with it > altogether. I'm inclined not to fix this at the moment - assuming that's not a > show-stopper for you - and address it in the context of a wider refactoring. > Any thoughts on this? It *is* a show stopper for me, I'm afraid: one of my EOL scripts just won't work if I can't set extended properties on a nested sequence. However, I don't mind maintaining the patched version of Epsilon at my Git mirror [1] myself, so there is no hurry. I think we could try WeakHashMap first and see if memory usage is kept low that way. If that doesn't work, then we could postpone the fix to a proper refactoring as you suggest. What do you think? [1]: http://j.mp/aScYSS Created attachment 172639 [details]
Patch for reusing the wrappers, using a WeakHashMap to save memory
For the record, trying to apply both fixes (reusing wrappers and trying methods with unwrapped values first) creates stability issues in EuGENia. It's an either/or situation. Since we discarded the approach which tried wrapped values first, I have replaced the two original patches with a new patch that changes EolTypeWrapper to reuse the wrappers, but stores them in a WeakHashMap so (hopefully) entries are garbage collected when they are no longer needed. I'll be testing this patch for a while and report any stability or performance problems. Is there any way I could stress test Epsilon automatically? Thanks for investigating. I'm currently working on eliminating wrapping altogether: I've got rid of EolInteger, EolString, EolBoolean and EolReal in my working copy and am starting to attack EolCollection and its subclasses - so we may have a clean solution for this by the end of the day... (In reply to comment #7) > For the record, trying to apply both fixes (reusing wrappers and trying methods > with unwrapped values first) creates stability issues in EuGENia. It's an > either/or situation. > > Since we discarded the approach which tried wrapped values first, I have > replaced the two original patches with a new patch that changes EolTypeWrapper > to reuse the wrappers, but stores them in a WeakHashMap so (hopefully) entries > are garbage collected when they are no longer needed. > > I'll be testing this patch for a while and report any stability or performance > problems. Is there any way I could stress test Epsilon automatically? This has been fixed in the SVN and in the latest interim version. Could you please update and confirm that this has been resolved? Created attachment 173623 [details]
Sample EOL script which shows a minor change in how EOL treats integers
(In reply to comment #9) > This has been fixed in the SVN and in the latest interim version. Could you > please update and confirm that this has been resolved? It works great, thanks! However, I'd like to note that there has been a slight change in how Epsilon handles integers. I have attached a sample EOL script which worked before removing the wrappers, and that doesn't work anymore unless the type declaration of the first argument is removed or changed to Integer. Basically, literal integers were implicitly converted into the EOL Real type before, but not any longer, so any operation which has a Real argument doesn't accept an integer anymore. I don't think it's a bug, but perhaps it should be noted in the release notes and/or the Epsilon book. Thanks for letting us know! I think it's easier for everyone if we just fix this and restore the old behaviour. I'll roll out a new interim version shortly. This should now be fixed in the SVN and the latest interim version available under http://download.eclipse.org/modeling/gmt/epsilon/interim/. Please let us know if you come across any other unexpected changes. Many thanks for your help!! Fixed in 0.9.0. |
Build Identifier: 20100218-1602 There are problems when unwrapping an object and then re-wrapping it for just one operation, just to remove the wrapping right after evaluating the expression. Namely, the extended properties that were set on the wrapper will be discarded. This is a problem when adding sequences to a sequence: by default, PointExecutor will attempt to perform the call using reflection and unwrapped types. This makes add() add an unwrapped element to the sequence, which later on shows the above problem after being wrapped again and again with different EolSequence instances. There are two possible solutions: * first try calling with wrapped values, and then try unwrapped values: this fixes this problem, does not lose any of the available API, and avoids the useless unwrapping and rewrapping that gets done here. But I might be missing something here: is there a reason why unwrapped values should be tried first, like now? * change EolTypeWrapper so that when wrapping an object that was wrapped before, the original wrapper will be returned. This uses more memory, but it might solve the problem in a more general way. I have attached a sample program which illustrates the problem: trying to set an extended property for the first nested sequence in a sequence works if it is built using Sequence{Sequence{1}}, but not if created as Sequence{} and then extended with add(1). I have also attached two patches which attempt to fix the problem in the above two ways. I'm not sure which one is better, but I think it's the first one, since it makes the example program print exactly the same thing for Sequence{Sequence{1}} and Sequence {} followed by add(1). Reproducible: Always Steps to Reproduce: 1. Run the attached EOL script. 2. Notice that the extended property is only properly printed when using Sequence{Sequence{1}}, and not when using Sequence{} and then add(1).