Ticket #8056 (closed: fixed)
Event comparison performance hit
Reported by: | Russell Taylor | Owned by: | Russell Taylor |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.0 |
Component: | Framework | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Peter Peterson |
Description
Changes in #7945 has a marked effect on a whole bunch of performance tests, and even showed up in some system tests (examples attached).
The cause of this needs to be found and reversed.
Attachments
Change History
Changed 7 years ago by Russell Taylor
- Attachment DataObjectsTest.EventListTestPerformance.test_compressEvents.runtime.v.revision.png added
Changed 7 years ago by Russell Taylor
- Attachment AlgorithmsTest.DiffractionFocussing2TestPerformance.test_SNAP_event_one_group_dontPreserveEvents.runtime.v.revision.png added
Changed 7 years ago by Russell Taylor
- Attachment SystemTests.ISISDirectInelastic.LETReduction.runtime.v.revision.png added
comment:1 Changed 7 years ago by Peter Peterson
- Status changed from new to inprogress
Re #8056. Making internal helper methods private.
Changeset: f571a4b3b20b6fed2d3c59ba5e35a41f79657c5d
comment:2 Changed 7 years ago by Peter Peterson
Re #8056. Making one helper function inline.
Changeset: 7e911d4da78caa863e197a767d558a3d6a7105a7
comment:3 Changed 7 years ago by Russell Taylor
I note that this last change recovered half the time previously 'lost' in EventListTestPerformance.test_compressEvents, but no change is observed in other affected tests such as EventListTestPerformance.test_integrate or RebinByPulseTimesTestPerformance.testExecution
comment:5 Changed 7 years ago by Russell Taylor
The full list of affected performance tests is:
- AlgorithmsTest.CropWorkspaceTestPerformance.test_crop_events_inplace
- AlgorithmsTest.DiffractionFocussing2TestPerformance.test_SNAP_event_one_group_dontPreserveEvents
- AlgorithmsTest.FilterByXValueTestPerformance.test_crop_events_inplace
- AlgorithmsTest.RebinByPulseTimesTestPerformance.testExecution
- DataObjectsTest.EventListTestPerformance.test_compressEvents
- DataObjectsTest.EventListTestPerformance.test_compressEvents_Parallel
- DataObjectsTest.EventListTestPerformance.test_integrate
- DataObjectsTest.EventListTestPerformance.test_sort_tof
comment:6 Changed 7 years ago by Nick Draper
Am I right in saying that this change only really affects the speed of CheckWorkspacesMatch? If so I think we should:
- move this ticket to the next release
- Remove the time taken to compare results from the performance tests
- Add one performance test just for testing the speed of CheckWorkspacesMatch.
comment:7 Changed 7 years ago by Russell Taylor
Re #8056. Inlined a number of methods for performance.
These methods were previously (implicitly) inlined. Moving their definitions out to the cpp had a negative impact on performance. I've moved the definitions back to the header file, but now they are explicitly inlined and defined outside of the class declaration.
Changeset: 370ae01444135f8ff0d778a039c3404a121b643e
comment:8 Changed 7 years ago by Russell Taylor
- Status changed from inprogress to verify
- Resolution set to fixed
All the affected tests listed above have recovered their performance (look here to see the DataObjects ones). There's one that we don't know about yet: DiffractionFocussing2TestPerformance only showed a change on the ISIS jobs, which is Ubuntu rather than RHEL but which looks at master instead of develop.
To test (branch is bugfix/8056_event_performance), look at the changes and see that they were just moving code, not changing it.
Note that none of this has anything to do with CheckWorkspacesMatch - it was just that they slowdown was introduced in a ticket that was doing stuff for that algorithm.
comment:9 Changed 7 years ago by Peter Peterson
- Status changed from verify to verifying
- Tester set to Peter Peterson
comment:10 Changed 7 years ago by Peter Peterson
- Status changed from verifying to closed
Merge remote branch 'origin/bugfix/8056_event_performance'
Full changeset: 56272a5876a7d60f3c7d7d0fb1b53382ccd564c5
comment:11 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 8901