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

[WIP] Pass coordinates by reference in 3D and 4D functions #1351

Closed
wants to merge 1 commit into from

Conversation

rouault
Copy link
Member

@rouault rouault commented Mar 23, 2019

This fixes cppcheck master warnings.
This could have a (likely tiny) performance advantage by saving useless
copies of the xyz/xyzt argument.

I should probably look at the differences in machine code in optimized builds. Passing by value as done currently might not perhaps be that bad on x86_64 if, as I presume values, might be stored in SSE registers and not passed on the stack.

This fixes cppcheck master warnings.
This could have a (likely tiny) performance advantage by saving useless
copies of the xyz/xyzt argument.
@rouault rouault changed the title Pass coordinates by reference in 3D and 4D functions [WIP] Pass coordinates by reference in 3D and 4D functions Mar 23, 2019
@kbevers
Copy link
Member

kbevers commented Mar 26, 2019

Is this still work-in-progress?

I don't think I understand the reason for this. What are the warnings that are avoided by this fix? It somehow seems a bit unnatural to me to pass by reference here, but I can't really explain why. Just one of those things, I guess :-)
Also, is it not necessary to give the 2D-functions the same treatment?

@rouault
Copy link
Member Author

rouault commented Mar 26, 2019

Is this still work-in-progress?

yes, until I manage find the motivation to look at the disassembly to see if that doesn't worsen things at least.

I don't think I understand the reason for this. What are the warnings that are avoided by this fix?

cppcheck (more recent versions than the one used in CI) warns as soon as a C++ struct/class instance is passed by value instead of reference. The rationale is that it saves copying of objects (passing by reference is under-the-hood similar as passing a pointer to the structure), and thus can help performance.

is it not necessary to give the 2D-functions the same treatment?

in theory yes, but cppcheck doesn't detect it (it is an inperfect tool). In the case of the 2D functions, the size of the XY/LP structure is just 16 bytes. Passing by reference on 64bit builds would be 8 bytes, so the gain would not be that much and basically all files under src/projections/ should be changed, so a lot of effort.

@kbevers
Copy link
Member

kbevers commented Mar 26, 2019

yes, until I manage find the motivation to look at the disassembly to see if that doesn't worsen things at least.

Right, doesn't sound like the most exciting thing to do :-)

The rationale is that it saves copying of objects (passing by reference is under-the-hood similar as passing a pointer to the structure), and thus can help performance.

Yes, I got the part about the performance, just not that that was the problem. It just sounded like a nice side effect. We have been getting a bunch of performance complaints lately (valid or not) so it may be a good idea to focus on that for a bit.

basically all files under src/projections/ should be changed, so a lot of effort.

This will have to be done sooner or later anyway[0], so we might as well coordinate the effort so that it only requires one pass.

[0] In the not-so-distant future all operations should be 4D, and 4D only.

@rouault
Copy link
Member Author

rouault commented Mar 26, 2019

so it may be a good idea to focus on that for a bit.

That would be just a micro-optimization. Nothing that I'd expect to have measurable consequences in performance profiles... The main focus was to have clean cppcheck output.

@busstoptaktik
Copy link
Member

That would be just a micro-optimization. Nothing that I'd expect to have measurable consequences in performance profiles

I did some timing checks some years ago, before introducing proj_trans().

Passing the PJ_COORD by reference made absolutely no difference on my setup at that time (I do not remember the details, but I am pretty sure that even on a PDP-11 the difference would be immaterial).

Also, the return value is returned by value, so some struct-sized copying is done anyway.

Copy link

@CherryKitty CherryKitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix to original content. I am not a coder. This is taking me litterally forever in a day to try and figure out. And preventing me from making art.

@rouault
Copy link
Member Author

rouault commented Oct 29, 2019

Dropping this PR. scripts/cppcheck.sh has been adapted to ignore the related warnings

@rouault rouault closed this Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants