Ticket #8056 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

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.

Change History

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:4 Changed 7 years ago by Peter Peterson

  • Owner changed from Peter Peterson to Russell Taylor

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:

  1. move this ticket to the next release
  2. Remove the time taken to compare results from the performance tests
  3. Add one performance test just for testing the speed of CheckWorkspacesMatch.
Last edited 7 years ago by Nick Draper (previous) (diff)

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

Note: See TracTickets for help on using tickets.