Community
Participate
Working Groups
It would seem more sensible if the SavingPolicy by default would just use resource.isModified(), enabled via setTrackingModification(true), instead of current (Sirius 1.0.0M5) default SavingPolicy which re-saves ALL models on saving ANY change... see http://blog2.vorburger.ch/2014/03/using-eclipse-sirius-with-lots-of-xtext.html
Actually when bug # is merged we won't need the setTrackingModification call. But yeah the main question is :what do we want as a default ? isModified is not reliable for all kinds of resource implementations for instance an XMI not using id's might trigger a different serialization in another XMI just by having one of it's objects moved. Knowing all the resources cross references and dependencies might help in being able to reduce the scope of what we need to save.
See also bug 426687.
A more sensible default strategy could be : check for isModified, any resource which has this flag might have serialization change (but may be no guarantees we'll have actually some) all the resources which have references to the ones being isModified might need to be serialized too as some URI fragments for these referenced elements might be different. We can filter a few known cases here too, if the isModified resource is using XMIIDs or is a CDO Resource then it won't happen. Now how can we compute the cross resources dependencies ? At worst we can getProperContents for each resource at save time, this has a complexity equivalent to eAllContents(). How would that compare to the current implementation ? Right now we are serializing all the resources just to make sure there is a change, and then serializing it again. Which in the ideal case of a "free" serialization overhead triggers two eAllContents. We could also maintain this list of cross resources dependencies if it makes sense but if only the "Save" require this it's probably not worth it to add a marginal cost on every change. On the other hand we might decide to serialize again this filtered list of Resource to make sure there is an actual change in the serialization if isModified might be true even if there is no serialization change. This is important for SCMs like clearcase so that we call the Eclipse Validation Edit APIs with all the resources at once and so that the end users have a popup to "checkout" only the files which are going to be changed. But then we might re-consider if the slightly degraded mode would not make a better default, at worst the end user will checkout a few more files than strictly necessary. I know some of the Obeo Designer users are using clearcase with a specific saving policy only checking the isModified because the "let's save everything" default behavior is not worthit to them compared to having false positives. As a conclusion : there is a lot of room for improvement which doesn't look like a huge impact on Sirius.
(In reply to Cedric Brun from comment #3) > We could also maintain this list of cross resources dependencies if it makes > sense but if only the "Save" require this it's probably not worth it to add > a marginal cost on every change. It's not the first time I feel that this kind of info, if it could be obtained (and maintained!) in an efficient and reliable way, would provide huge performance benefits in several areas. However, every time I started to think more concretely, I soon found that it would be rather costly to do it correctly. Note that GMF's CrossReferenceAdapter for example maintains such a list of resource import/exports, but at a cost that we considered high enough that we are in the process of removing it from Sirius (see bug #427017). Maybe this relatively high cost would be worth if it was paid only once and used to used to optimize several features. If it were abstracted in a generic enough API it could possibly be optimized itself at some later point.
Moving to M7, at least to reconsider what the default behavior should be.
Changed title/summary to something shorter.
This issue and Bug 426687 might have impacts on each others.
I pushed a proof of concept here : https://git.eclipse.org/r/#/c/25053/ As Maxime said, it is Bug 426687 and this patch *assume* that isModifed is managed in some way for all the resources in the resourceset. One idea to test this, would be to plug it alongside the current policy and launch all the tests with both, checking that the list of resources to be saved which are found by both implementation is always the same. Not sure if the "multi-resources" + "multi kind" (using UUIds or not) are very well covered by the tests though. From a performance POV, this effectively avoid to save the resources which have not changed or do not depend on changed resources, cutting the cost of a save operation *at least* in two but most of the time by an order of magnitude.
Moving to 1.0.0, there is not enough time left before M7 for such an potentially impacting change.
The current patch causes important regressions in our internal test suites (both using Cédric's original version or mine). I'm investigating, but if I can't stabilize it before monday, I think this will be moved to 2.0.0, or maybe added as an available alternative "experimental" saving policy that people can opt-in, but not enabled by default.
I have pushed 2 commits related to this issue: - http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=b5630af25c241a3a7ad974b9c29171bb614e04df: simple refactoring which introduces a new abstract class with code that can probably be shared between most saving policies (a separate commit will mention the change in the release notes, see https://git.eclipse.org/r/#/c/27266/). - http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=2d25e2d4006b2056385056bc8e03ff1df9b83c45: adds the new IsModifiedSavingPolicy, but keep it internal and do not make it the default. I was not able to fully understand the regressions introduced by the new implementation, and it is too late not for 1.0.0, so I'm moving this ticket to 2.0.0. The code is available (although in an internal package), so people who want to try it can set it up explictely using Session.setSavingPolicy(), and help us fix the remaining issues for 2.0.0.
Created attachment 243473 [details] Consistency check to help debug differences between the old and new saving policies This patch can be used to help debug the differences between the old an new policies. It applies the old saving policy, but also invokes the new one and outputs some diagnostic messages if their results would differ.
Small update to report progress on this ticket. Thanks to the patchsets of Pierre-Charles I could test the whole set of tests (junit & swtbots) with both implementations working together to compare the result and detect cases where the "new" implementation would not save a resource the "old" implementation would have picked. This was not without hurdles, I had to change the ResourceSaveDiagnose implementation to reload the filesystem version so that I can compare both serialization without being impacted by serialization options like LINE_DELIMITER and the "old" implementation actually had the side effect of setting all the resource.isModified to false when serializing in a buffer, leading to the new implementation deciding to do nothing. Anyway, once this has been done I could identify that : - the current policy doesn't care if Resource.isLoaded is true or not. The new one should not then. - the new one should make sure to save the resource content in the case where the file does not exist *yet* on the filesystem. The ResourceSetSync would then return the status : UNKNOWN. With these two changes we have 21 cases where the "new policy" would not save a resource which the "old policy" would have saved, 2569 cases where both policies are doing the same decision and 879 where the new policy will save slightly more than what is strictly necessary (but that's the tradeoff we are willing to make) of the remaining 21 cases, it looks like failures are coming from tests related to model migration and indeed it makes sense, a model might have been migrated during loading and then has not "isModified=true", hence will not be saved. It used to be with the old policy because then it would not have the same serialization. Setting setModified(true) in this case doesn't look like a good option are we are *in the process of loading* when we know if a migration is necessary or not and once we are loaded setModified() will be set to false. (and it looks like a hack one might easily destroy without realizing why). I am testing an approach where an adapter is added on the Resource to "mark" it has having been migrated so that the new policy will decide to serialize it anyway. Tests are under way, for reference the branch and commits are here : https://github.com/cbrun/org.eclipse.sirius/tree/isModifiedSaving_430724_testbranch .
Commit 1875d3ccc230d770cc94607ca852a517fc6fe4f2 Down to 16 problematic cases in two testcases. org.eclipse.sirius.tests.swtbot.table.SetPropertyOfTableTreeByPropertiesViewTest where an .ecore file is not getting saved by the new policy whereas the old would have. (4 differences) org.eclipse.sirius.tests.unit.common.migration.DiagramMigrationTestCampaign03 where TestCampaign_03.migrationmodeler will not be saved by the new one thought would have been saved by the old one. (12 differences)
org.eclipse.sirius.tests.unit.common.migration.DiagramMigrationTestCampaign03 Looks like the test itself is buggy. The model it is using is not even loadable in the first place. The new saving policy will not save it, the old would have because then the serialization would have changed. I would say the new behavior will not systematically cause a loss of data and should be kept.
A small update to report progress on this ticket. After many debugging session it appeared that in several places in the Sirius code, session.save() is called within the context of a read-write transaction and that even with the commit from Pierre-Charles which was trying to "handle" this by listening to isModified=false events this introduces man indeterminisms. It makes no sense to *save* in the middle of a transaction and is only asking for more troubles (triggers are not launched yet, we might be saving an half modified model, dangling references might still be presents and so on.) I witnessed many cases where SiriusControlTests would have different outcome without any change in the code or in the target platform (and AFAIK it is one of those "unreliable" tests we have) So I started by this : any session.save() done during a transaction will postpone the save operation up to the next post-commit phase. That is indeed a subtle change but it makes things determinists. Once this was done SiriusControlTests became reliable. Do you want me to open a new ticket for this change in itself ? The isModifiedSavingPolicy will require it anyway, right now it has its own commit. I also refined the way we detect resource inter-dependencies and made sure the policy would not process non platform:/resource resources (you'll find details in the commit messages) All in all, testing this hybrid approach with the reverse-10-times projects : - open the project, open a diagram, move something around, save => only the .aird file is getting saved, the whole operation goes from 60 seconds to 17 seconds (the .aird file is about 80Mo) Tests results : no regression on junit, swtbot & sequence. Another (potential) good news. On my testdrive run, the test execution was about 30% faster than before (and that would make sense based on the actual changes) But there are many other parameters like the server load so we might only be sure with many runs. I reworked the commits, a request for review might come quite soon, in the meantime they are still here : https://github.com/cbrun/org.eclipse.sirius/commits/isModifiedSaving_430724_testbranch down to 4 commits which are quite small.
Changes available for review : https://git.eclipse.org/r/#/c/34844/ : improvements in the way the IsModifiedSavingPolicy works https://git.eclipse.org/r/#/c/34845/ : introduce a marker to make sure we handle the silently migrated models https://git.eclipse.org/r/#/c/34846/ : activate this saving policy be default (which we might or might not want to do just yet, it sounds safer to make sure we use it in the Xtext context - which is the original report) and activate it by default as soon as the 3.0 development starts.
I've merged the first two patches from Cédric (commits 5f2a8553449d17511eda6dbbe946da784513204e and cd19557308cf2be353a0f54ffaa8f26417c99091), but it is not enabled by default for now. For 2.0.0 we will enable it only when the Xtext bridge is installed; even if it was rather thoroughly tested by Cédric, it's rather late in the cycle to introduce such a change. We should probably mention it in the N&N for the release, so that people who do not use Xtext but experience long save time know that they can opt-in to use the new policy and help us stabilize it even more. It should become the new default on the master branch as soon as 2.0.0 is released so that we find any issue with it as soon as possible by forcing the dev team to use it (with fixes/improvements made on master backported to 2.0.x when possible).
The new saving policy is now the default when the Xtext integration plug-in is installed, see commit f6e0cbcafe056697ba657dcf8af5c05479687698. If time permits I'll see if we can merge code inspired by Cédric's comment on https://git.eclipse.org/r/#/c/35127/ to only enable the new policy if there are Xtext resources. As it is, just *installing* the Xtext bridge will enable the new policy for all Sirius sessions.
(In reply to Pierre-Charles David from comment #19) > The new saving policy is now the default when the Xtext integration plug-in > is installed, see commit f6e0cbcafe056697ba657dcf8af5c05479687698. > > If time permits I'll see if we can merge code inspired by Cédric's comment > on https://git.eclipse.org/r/#/c/35127/ to only enable the new policy if > there are Xtext resources. As it is, just *installing* the Xtext bridge will > enable the new policy for all Sirius sessions. Done with https://git.eclipse.org/r/#/c/35296/.
Created attachment 248097 [details] A modeling project with 5000 Xtext resources and a VSM project based on statemachine Attaching a Modeling project with 5000 Xtext resources containing statemachines. With the latest improvements and the use of the IsModifiedSavingPolicy by the Xtext bridge the save time went down to a few seconds when a few files are changed. We could reduce that time further by disabling the "safeguard" checks which prevents any serialization to be done in the workspace if there is no actual change. To reproduce the test case you will need the Statemachine DSL installed (or in a development workspace) https://bugs.eclipse.org/bugs/attachment.cgi?id=248092
(In reply to Cedric Brun from comment #21) > Created attachment 248097 [details] > A modeling project with 5000 Xtext resources and a VSM project based on > statemachine > > Attaching a Modeling project with 5000 Xtext resources containing > statemachines. > > With the latest improvements and the use of the IsModifiedSavingPolicy by > the Xtext bridge the save time went down to a few seconds when a few files > are changed. So it looks like the original use case (Xtext) is fixed, but the ticket was worded in a more general way, and the new policy is still not the default for 2.0. Should we: 1. keep this ticket opened and move it to 3.0 where we want the new policy to because the default; 2. close this one (considering the Xtext original case is now OK), and create a new one for 3.0 to change the default?
I would go for "option 2" to at least document correctly that the Xtext case is now handled with 2.0.
The original use case (Xtext) is now fixed. See bug 448655 for the followup work to make the new and improved policy the global default.
Verified on Sirius 20141024-094306
Available in Sirius 2.0.0.
