Bugzilla – Full Text Bug Listing
| Summary: | Performance of UML definition look-up in static profiles | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] MDT.UML2 | Reporter: | Christian Damus <give.a.damus> | ||||||||
| Component: | Core | Assignee: | Christian Damus <give.a.damus> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | Kenn Hussey <Kenn.Hussey> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P2 | CC: | benoit.maggi, johan, klaas.gadeyne, papyrus-bugs | ||||||||
| Version: | 5.2.0 | Flags: | Kenn.Hussey:
neon+
|
||||||||
| Target Milestone: | SR2 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 498862 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Christian Damus
I have patches in hand to propose for the Neon maintenance branch (no API changes) and Oxygen/master branch (also no API changes but different because of the work for bug 497359). Created attachment 264261 [details]
Patch for 5.2.x maintenance branch
Attached a patch on the 5.2 maintenance branch. This patch does not change any API.
The new JUnit test doesn't really test performance, but it logs some statistics that could be monitored over time. In particular, running the test without the UMLUtil changes in this patch results in something like these timings on my Mac:
Reading: 105.38 ms (σ = 12.761 ms) on 10 iterations
Creation: 581.50 ms (σ = 20.417 ms) on 10 iterations
where the first test scenario is strictly reading stereotype applications (as in the SysML 1.4 diagram refresh use case) and the other is a scenario involving a lot of model editing (which continually clears the CacheAdapter).
With the UMLUtil changes in the patch, we get these timings:
Reading: 10.25 ms (σ = 2.605 ms) on 10 iterations
Creation: 509.63 ms (σ = 7.328 ms) on 10 iterations
So, a considerable improvement in the model reading scenario but apparently also some slight improvement in model editing, although in the latter case it's more interesting that the individual data-points are more consistent.
Created attachment 264262 [details]
Patch for the master (Oxygen) branch
This patch ports the maintenance-branch fix to master, accounting for the other change already made that introduces a new caching API for scenarios that also modify the model (which would invalidate the CacheAdapter) where it is known that such changes would not affect profile/stereotype metadata.
As in the other patch, the JUnit tests measure some timings that could be monitored from release to release. The timings without the fix in the UMLUtil class are, again on my Mac, something like:
Reading: 101.13 ms (σ = 1.727 ms) on 10 iterations
Creation: 587.88 ms (σ = 23.204 ms) on 10 iterations
And with the UMLUtil patch:
Reading: 9.88 ms (σ = 0.641 ms) on 10 iterations
Creation: 591.13 ms (σ = 10.855 ms) on 10 iterations
So, again, a marked improvement in the model reading scenario and similar performance in the model mutation scenario. Interestingly, most runs that I do in my development environment show a smallish increase in the creation scenario as shown here, but always a considerably narrower deviation. I don't expect that this systematic increase for this (fairly unrealistic) modification scenario should be of concern.
One more note on the JUnit tests: this fix obviates the performance advantages gained by use of the UMLUtil::executeOperation(Runnable) API for purely model reading operations, so it was necessary to delete the test case measuring the performance of that scenario in the new API.
Thanks, Christian, I'll take a look as soon as I am able to. Note that this fix just missed the Neon.1 release train (as we are now in the "quiet week" for that release), so it will be targeted for Neon.2 (and Oxygen). (In reply to Kenn Hussey from comment #4) > Thanks, Christian, I'll take a look as soon as I am able to. Note that this > fix just missed the Neon.1 release train (as we are now in the "quiet week" > for that release), so it will be targeted for Neon.2 (and Oxygen). Yes, that's fine. Papyrus is in the same boat, so we're targeting Neon.2 anyways (it's before year end) Thanks! Also, to be absolutely clear, this in no way supersedes/obviates/invalidates the new API that we added for bug 497359. That is still necessary to have a caching mechanism outside of the CacheAdapter that is unaffected by changes to the model that invalidate the adapter's cache. That is critical to some of our transformation use cases! (In reply to comment #2) > Created attachment 264261 [details] > Patch for 5.2.x maintenance branch > > Attached a patch on the 5.2 maintenance branch. This patch does not change any > API. I just looked at this patch. Would it be better to look for the existence of a key for a given named element instead of retrieving the value and comparing with null, to avoid multiple lookups for elements that have no definition? (In reply to Kenn Hussey from comment #6) > > I just looked at this patch. Would it be better to look for the existence of > a key for a given named element instead of retrieving the value and > comparing with null, to avoid multiple lookups for elements that have no > definition? The way it's coded is optimized for the elements that do have definitions. Is it not more usual that there is a definition? And if there isn't, then something is broken, so performance wouldn't really matter in that case, right? (In reply to comment #7) > (In reply to Kenn Hussey from comment #6) > > > > I just looked at this patch. Would it be better to look for the existence of > > a key for a given named element instead of retrieving the value and > > comparing with null, to avoid multiple lookups for elements that have no > > definition? > > The way it's coded is optimized for the elements that do have definitions. Is > it not more usual that there is a definition? And if there isn't, then > something is broken, so performance wouldn't really matter in that case, right? Yes, there's usually a named element for a given definition, except in cases where a type has been removed from a newer version of a profile (for example), which should rarely/never be the case given best practices. The fix has been committed/pushed to the 'R5_2_maintenance' branch in git and will be available in the next published build for UML2 5.2.2. The fix is available in a maintenance build for UML2 5.2.2. I'll look at the patch for UML2 5.3.0 next. OK, I've finally had a chance to review the patch for the 'master' branch (sorry for the delay). I think it looks good, although the comment for the NULL operation context should be updated because it doesn't just always calculate the results anymore (and hence should perhaps even be renamed from "NULL" to "DEFAULT" accordingly). (In reply to Kenn Hussey from comment #10) > OK, I've finally had a chance to review the patch for the 'master' branch > (sorry for the delay). I think it looks good, although the comment for the > NULL operation context should be updated because it doesn't just always > calculate the results anymore (and hence should perhaps even be renamed from > "NULL" to "DEFAULT" accordingly). Thanks, Kenn, for the review! I agree with your comments; definitely an oversight. Expect a new patch, soon. Created attachment 264799 [details]
Updated patch for the master (Oxygen) branch
I've attached a new patch for the master branch to address the code review comments.
Comment on attachment 264799 [details]
Updated patch for the master (Oxygen) branch
Looks good, Christian. Did you want to commit/push the changes or shall I?
(In reply to Kenn Hussey from comment #13) > Comment on attachment 264799 [details] > Updated patch for the master (Oxygen) branch > > Looks good, Christian. Did you want to commit/push the changes or shall I? Thanks for your help, Kenn! I have pushed the commit to master. Christian, I'm seeing an intermittent failure of org.eclipse.uml2.uml.bug.tests.Bug497359Test.testReapplyProfile now that the changes have been merged to master. See https://hudson.eclipse.org/uml2/job/mdt-uml2-master/268/testReport/junit/UML%20Tests%20stand-alone$UML%20Bug%20Regression%20Tests$Bug%20497359%20tests$org.eclipse.uml2.uml.bug.tests/Bug497359Test/testReapplyProfile/ for an example. Could you take a look? (In reply to Kenn Hussey from comment #15) Mm, it appears that I have improved the baseline performance of working with profiles too much. ;-) (In reply to Christian W. Damus from comment #16) > (In reply to Kenn Hussey from comment #15) > > Mm, it appears that I have improved the baseline performance of working with > profiles too much. ;-) The test is fixed with commit d991e71bd846074283d32e645567e1ad9f10a16e. The fix is now available in an integration build for UML2 5.3.0. |