Bugzilla – Full Text Bug Listing
| Summary: | [Performances - Model Explorer] really slow with a big model | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] Papyrus | Reporter: | Mathieu Velten <mathieu.velten> | ||||||||
| Component: | Core | Assignee: | Project Inbox <mdt-papyrus-inbox> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | alain.leguennec, cletavernier, faure.tristan, papyrus-bugs, tomas.sandkvist, yann.tanguy | ||||||||
| Version: | 0.8.0 | ||||||||||
| Target Milestone: | SR1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 441857 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Mathieu Velten
fixed. don't compute the delete command for isEnabled. backported on trunk What is a "big model" ? Mathieu can you add an example to reproduce, please. No unfortunately I can't, the model is confidential. in terms of metrics the model is around 50 mo. Calculating a delete command on a high level package is REALLY long since it chains the delete commands of all the underlying elements using the element types framework. I don't really see the point of computing the whole command just to hide the delete command anyway, isEnabled should be simple enough since it is called very often. I understand the performance issue, but having delete always enabled may not be ideal either. There can be items in the tree for which delete should not be enabled (read-only model elements, or LinkItem/EReference). So what about delay the check and display a dialog box if the delete is not possible ? Created attachment 201993 [details]
Proposed fix for the regression
(1) I, Alain LE GUENNEC, wrote 100% of the code I've provided.
(2) This code contains no cryptography
(3) I have the right to contribute the code to Eclipse.
(4) I contribute the content under the EPL.
Mathieu, you actually did introduce a significant regression when changing the DeleteCommandHandler: Now delete will only work once (until the DeleteCommandHandler is disposed/renewed at least). This is due to the fact that in the previous version, isEnabled() used to reconstruct the command each time the selection changed. Now it does no more. The command is now built only once, during the first call to getCommand(), that is when the delete command is first executed. After that, the subsequent calls to execute() will call getCommand() again, which will return the old cached command from the first delete execution, instead of building a new command, and since the element is already deleted, it probably does nothing. The fix is simple: Just don't cache the command anymore (it's now only used during execute() and no more during isEnabled(), so there's no point in caching it anymore). See attached patch. oups, sorry for that. what do you think about my previous suggestion to "replace" the isEnabled by a dialog box ? fix commited on trunk and 0.8 branch. I am afraid that a popup dialog when delete cannot be executed would be a bit disruptive to the users.
Moreover, unless you provide a rationale for the delete failure in a descriptive error message, it would not bring much compared to doing nothing. And building that descriptive error message might not be trivial.
I really think isEnabled should be smarter than just returning true, while still not constructing the full delete command, so that the number of cases where delete is enabled and yet will fail would be small enough.
Something to consider would be to build a minimal ("lazy") command in "setEnabled", which would allow isEnabled() to call canExecute() efficiently, and still have everything fit in the command framework (ie edithelper/advices would still be called, for instance).
Maybe this initial phase could avoid adding deletion of dependents graphical elements to the command for example, assuming that diagrams will never prevent deletion of semantic elements.
The rest of the delete command would then be built on-the-fly during execute()..
Don't know if that's easy to do, though..
Note that the Paste command also suffers from similar performance issues, although it's less apparent (but on big models, this can be observe too).
You can find attached a patch which check for read-only on all the resources associated with sub elements of the element. This looks ok performance wise (the read-only state of a resource is cached in the EditingDomain, which help a lot I guess). You mention LinkItem/EReference, but the delete doesn't seem to be registered on it. Can you think of other use cases ? Created attachment 202312 [details]
Read-only check
Created attachment 202313 [details]
mylyn/context/zip
By the way, I noted a similar design choice (always enabling, only checking at execution whether it is actually possible to proceed or not) in another place: org.eclipse.papyrus.navigation.CreateDiagramWithNavigationHandler. I just noticed that issue, because until now we were still using the old deprecated org.eclipse.papyrus.modelexplorer.actionprovider.CreateDiagramActionProvider in SCADE System. When switching to the new mecanism which uses command handlers based on CreateDiagramWithNavigationHandler, I noticed that basically all diagrams are always enabled on all kinds of elements, and instead you get an "It is not possible to create this diagram on the selected element" error popup when it should not have been enabled in the first place. I am not too sure what it means to navigate to other elements upon creating a new diagram (I guess there must be a use-case for that), but it really is annoying that the UI now provides no pro-active feedback to the user as to which kinds of diagrams can be created in which context. Since this old bug is still open: Is the Model Explorer fast with a big model today? It's much faster today (Since Kepler SR1), but we've recently (yesterday) noticed a performance degration when using a big model with many stereotypes. I doesn't have too much impact on the ModelExplorer itself, though. So I'd say yes, the ModelExplorer is much much faster now on big models, except for a few corner cases (Which are being fixed). OK, but the bug has been in state REOPENED for two years, so someone should perhaps define an few test cases and a "definition of done". Commits ba4827633bf0d405aa719670a9a66afc070b26a6 (Master) and 78abd37a54727ae31df5c56b68279fb51d23e1a1 (0.10-maintenance) introduce an optimization of Model Explorer queries. These queries used to rely on a cross-referencer to find all diagrams/tables/etc. The new implementation only browses the roots of notation resources (+ di resource for Tables v1) Commits 9b4af1c8024448f987e6fb93857f80e3b3767798 (Master), 2c9eb3b28789cec241eb4e99e6a52e723c4fd606 (0.10-maintenance) The previous patch was incomplete (One of these queries had not been patched) I would say that the Model Explorer still is almost unbearingly slow. I have a model split into 50 submodels, and two profiles (perhaps 20-30 stereotypes defined). And it takes about 2-3 second to jump from package to package if I open the "top" model. So yes, that is slow. Adding a package, edit the name and save takes more or less at least a minute, maybe longer. I am using the 0.10.1 version (I guess the only one available) Not exactly ; 0.10.2 and 1.0.0 are in development. Fixes published after September 2013 are only available in these development versions. Nightly builds can be found here: http://www.eclipse.org/papyrus/updates/index.php 0.10.2 will be released on February 2014. 1.0.0 will be released on June 2014. Both versions will include these performance fixes. And indeed, 0.10.1 still suffers major performance issues. Note that nightly builds are not recommended for production; only for testing the very latest features and bug fixes. (In reply to Tomas Sandkvist from comment #21) A severe performance bug related to larger models was fixed before Xmas, plus some minor performance issues. Can you run your test case again and check if adding a package etc. still takes ~ a minute using the latest nightly build? Thanx/Toni I have now tested build 0.10.1.v201401100619 and 0.10.1.v201401100559, those version was what I got when I used the update site http://download.eclipse.org/modeling/mdt/papyrus/updates/nightly/kepler, don't know if that is the latest/best (sent an email to Toni and asked, didn't get a reply before I did get the opportunity to test). And Papyrus is definitely more responsive now. I have tried all the situations where I have found it to begin lagging, requiring a re-opening of the model, and what ever I do it seems to be really, really nice, responsive and lightning fast! I think you nailed it! Great work! Do you want be to try another version, just ask! Best regards, Tomas |