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

Avoid calling msProjectionsDiffer over and over again #5259

Closed
wants to merge 1 commit into
base: branch-7-0
from

Conversation

Projects
None yet
3 participants
@tbonfort
Member

tbonfort commented Mar 1, 2016

As reported on the mailing list in http://osgeo-org.1560.x6.nabble.com/Performance-issue-with-MS-7-x-td5253419.html , there is a performance issue that became visible once the code to msProjectionsDiffer became more complicated with eeb1e78. This PR aims to set layer->project when necessary and then avoid calling msProjectionsDiffer over and over (for each feature)
cc @pmauduit @yjacolin

@yjacolin

This comment has been minimized.

Show comment
Hide comment
@yjacolin

yjacolin Mar 1, 2016

Contributor

we will test this soon. Thanks!

Contributor

yjacolin commented Mar 1, 2016

we will test this soon. Thanks!

@pmauduit

This comment has been minimized.

Show comment
Hide comment
@pmauduit

pmauduit Mar 3, 2016

Contributor

Thanks @tbonfort ! This is way better after your commit (tbonfort@7cb9582):

here are some metrics about my own tests, always using the same query:

/path/to/mapserv QUERY_STRING="MAP=./mymapfile.map&SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&FORMAT=image%2Fpng&TRANSPARENT=true&LAYERS=may_layer&CRS=EPSG%3A3857&STYLES=&WIDTH=2036&HEIGHT=745&BBOX=-13071343.332991421%2C885446.5356554817%2C6848757.734351791%2C8174481.552929889"

Time needed before your commit (HEAD^ on your branch):

real    0m5.282s
user    0m5.095s
sys 0m0.188s

Time needed after:

real    0m1.985s
user    0m1.846s
sys 0m0.132s

Callgrind analysis before your commit:
callgrind-before-tbonfort

~38.5 billions CPU cycles needed, most of the time wasted in projections init (pj_init_ctx)

Callgrind after the commit:

callgrind-tbonfort

~11.8 billions CPU cycles needed.

The previous version used by our customer was still faster than the 2 studied here (only 6.5 billions CPU cycles were needed for the previous query in Mapserver 6.4), but the current optimization is clearly better than nothing.

Contributor

pmauduit commented Mar 3, 2016

Thanks @tbonfort ! This is way better after your commit (tbonfort@7cb9582):

here are some metrics about my own tests, always using the same query:

/path/to/mapserv QUERY_STRING="MAP=./mymapfile.map&SERVICE=WMS&VERSION=1.3.0&REQUEST=GetMap&FORMAT=image%2Fpng&TRANSPARENT=true&LAYERS=may_layer&CRS=EPSG%3A3857&STYLES=&WIDTH=2036&HEIGHT=745&BBOX=-13071343.332991421%2C885446.5356554817%2C6848757.734351791%2C8174481.552929889"

Time needed before your commit (HEAD^ on your branch):

real    0m5.282s
user    0m5.095s
sys 0m0.188s

Time needed after:

real    0m1.985s
user    0m1.846s
sys 0m0.132s

Callgrind analysis before your commit:
callgrind-before-tbonfort

~38.5 billions CPU cycles needed, most of the time wasted in projections init (pj_init_ctx)

Callgrind after the commit:

callgrind-tbonfort

~11.8 billions CPU cycles needed.

The previous version used by our customer was still faster than the 2 studied here (only 6.5 billions CPU cycles were needed for the previous query in Mapserver 6.4), but the current optimization is clearly better than nothing.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Mar 3, 2016

Member

I wold say that something else has changed then, because with this fix we will end up calling msProjectionsDiffer much less often than in 6.x . Can you show the full callgrind output for 6.4 ?

Member

tbonfort commented Mar 3, 2016

I wold say that something else has changed then, because with this fix we will end up calling msProjectionsDiffer much less often than in 6.x . Can you show the full callgrind output for 6.4 ?

@pmauduit

This comment has been minimized.

Show comment
Hide comment
@pmauduit

pmauduit Mar 3, 2016

Contributor

Using a compiled version of MS on branch-6.4, I actually have the same result as the one after your commit:

callgrind-64

But below is the screenshot I obtained last week when the same query was performed on the official debian jessie package ; there might be some optimizations done at compilation time by the debian packaging team.

callgrind-641-vanilla

For the record, here is the full debian package version:

# apt-cache policy cgi-mapserver
[...]
     6.4.1-5+b3 0
        500 http://httpredir.debian.org/debian/ jessie/main amd64 Packages
Contributor

pmauduit commented Mar 3, 2016

Using a compiled version of MS on branch-6.4, I actually have the same result as the one after your commit:

callgrind-64

But below is the screenshot I obtained last week when the same query was performed on the official debian jessie package ; there might be some optimizations done at compilation time by the debian packaging team.

callgrind-641-vanilla

For the record, here is the full debian package version:

# apt-cache policy cgi-mapserver
[...]
     6.4.1-5+b3 0
        500 http://httpredir.debian.org/debian/ jessie/main amd64 Packages
@pmauduit

This comment has been minimized.

Show comment
Hide comment
@pmauduit

pmauduit Mar 4, 2016

Contributor

@tbonfort do you think it can be merged, or do you still need to work on this PR ? (or any extra infomation from my side ?)

Contributor

pmauduit commented Mar 4, 2016

@tbonfort do you think it can be merged, or do you still need to work on this PR ? (or any extra infomation from my side ?)

tbonfort added a commit that referenced this pull request Mar 4, 2016

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Mar 4, 2016

Member

rebased into branch-7-0 in 6acfaf4

Member

tbonfort commented Mar 4, 2016

rebased into branch-7-0 in 6acfaf4

@tbonfort tbonfort closed this Mar 4, 2016

@tbonfort tbonfort added this to the 7.0.2 Release milestone Mar 4, 2016

@tbonfort tbonfort deleted the tbonfort:msProjectionsDiffer-perf branch Mar 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment