Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor SofQWNormalisedPolygon for performance #12584

Closed
eXeC64 opened this issue May 12, 2015 · 1 comment
Closed

Refactor SofQWNormalisedPolygon for performance #12584

eXeC64 opened this issue May 12, 2015 · 1 comment
Assignees
Labels
Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period.
Milestone

Comments

@eXeC64
Copy link
Contributor

eXeC64 commented May 12, 2015

This issue was originally TRAC 11746

Running the script below seemed very slow so I profiled the execution.

source_file = 'MAP05935_ei34.nxspe'
mat_ws = 'MAP05935'
Load(Filename=source_file, OutputWorkspace=mat_ws)
SofQW(InputWorkspace=mat_ws, OutputWorkspace='out', QAxisBinning='0,0.02,5', EMode='Direct', Method='NormalisedPolygon')

Valgrind indicates that 30% of the execution time is being spent handling thrown exceptions. This comes from the call in SofQNormalisedPolygon::exec to Rebin2D::rebinToFractionalOutput which calls intersectionByLaszlow in a tight loop. intersectionByLaszlow returns either a ConvexPolygon representing the intersection, or throws an exception. We're throwing a huge number of exceptions in a tight loop, which is very bad for performance.

Instead intersectionByLaszlow should accept an additional non-const ConvexPolygon reference that will be used for output, and then return a bool indicating whether an overlap exists. This should reduce the execution time by 30%.

In addition, there is a strongly suspected secondary performance problem: Vertex2D. Vertex2D is a V2D like class that implements its own linked list. It's used by ConvexPolygon to represents the polygon's vertices. During the execution of the algorithm we allocate and deallocate thousands of Vertex2Ds on the heap, which cannot be good for the algorithm's performance. We should strongly consider removing Vertex2D from Mantid in totality, and in this case use std::vector`` instead.

Overall this ought to improve performance by 40%, if not more. A performance test should be created before refactoring work begins, to allow measuring any performance improvements made.


Keywords: maintenance

@eXeC64
Copy link
Contributor Author

eXeC64 commented May 26, 2015

@martyngigg (2015-05-26T09:17:23):
Thanks for the profile information. I had always wondered whether the tight-loop exception handling was an issue and it's good to have some hard numbers to say so.

I think this will probably split into 2 tickets as the it will be worth reprofiling after the exception changes to see whta difference the Vertex2D changes can make. A lot of that code was borrowed from another place that implemented the algorithm.

@eXeC64 eXeC64 added Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period. labels Jun 3, 2015
@eXeC64 eXeC64 added this to the Release 3.5 milestone Jun 3, 2015
martyngigg added a commit that referenced this issue Jun 20, 2015
martyngigg added a commit that referenced this issue Jun 20, 2015
martyngigg added a commit that referenced this issue Jun 24, 2015
The points are now stored in a container rather than a custom
linked-list. This will cut down on the the operations required to
insert a new node.
Refs #12584
martyngigg added a commit that referenced this issue Jun 24, 2015
martyngigg added a commit that referenced this issue Jun 24, 2015
This is just a simple forward iterator that doesn't implement the full
iterator interface.
Refs #12584
martyngigg added a commit that referenced this issue Jun 24, 2015
martyngigg added a commit that referenced this issue Jun 24, 2015
PoylgonIntersection uses new style of creating polygons.
Refs #12584
martyngigg added a commit that referenced this issue Jun 24, 2015
martyngigg added a commit that referenced this issue Jun 24, 2015
Initial tests suggest this has given a factor of 2 improvement in speed.
Refs #12584
martyngigg added a commit that referenced this issue Jun 25, 2015
martyngigg added a commit that referenced this issue Jun 25, 2015
Clang and MSVC both require the full definition to be able to use it
as a member variable.
Refs #12584
martyngigg added a commit that referenced this issue Jun 25, 2015
martyngigg added a commit that referenced this issue Jun 25, 2015
martyngigg added a commit that referenced this issue Jun 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
None yet
Development

No branches or pull requests

2 participants