Opened 17 years ago

Closed 17 years ago

Last modified 17 years ago

#1244 closed defect (fixed)

[Performance] PointObj cause performance lost

Reported by: jlacroix Owned by: mapserverbugs
Priority: high Milestone: 4.6 release
Component: MapServer C Library Version: unspecified
Severity: normal Keywords:
Cc: jmckenna@…, sgillies@…, fsimon@…


In bug 1224, I found that that a performance lost is observed since we add the Z
parameter in the point object. Just because the pointObj is bigger, it takes
longer to access the parameters of the pointObj.
A line like:
      x1 = shape->line[i].point[j].x;
      y1 = shape->line[i].point[j].y;
takes between 15% and 50% more time.

Daniel and I talked about it and he proposed to put all access to the m and z
parameters inside a 
#ifdef USE_SHAPE_Z_M

By default we could make the m and z options disabled since most of the users
don't use it.

I made some test and it seems that if we put the m and z inside the ifdef, we
gain with this change:
(50 calls to the gmap mapfile)
With the M and Z parameters in point object
4.4: 7.56
4.0: 6.18

With all M and Z parameters inside not enabled ifdef
4.4: 7.10
4.0: 6.23

I propose to commit that in 4.5, but this would require changes to the core and
to mapscript (all flavor). So I want to inform you and maybe have some comment

Change History (21)

comment:1 Changed 17 years ago by jlacroix

blocked: 1224

comment:2 Changed 17 years ago by sdlime

Hmmm... I thought z and m values were stored at the shapeObj level rather than 
at the pointObj level although I can understand why you might want to do both. 
I hesitate to add yet another compile time option.


comment:3 Changed 17 years ago by woodbri@…

I would think the the compiler optimization should deal with the
"shape->line[i].point[j]" as a semi-invariant within the loop, but maybe it is
not. You should try some manual optimization in case the expression is too
complicated for the compilier to optimize correctly. Like:

 pobj = shape->line[i].point[j];
 x1 = pobj.x;
 y1 = pobj.y;

comment:4 Changed 17 years ago by jlacroix

Steve, I know we already have a lot of compile option, but How can we fix that
if not by a compilation option?

We may move the m and z parameters alone in a new object (PointDimensionObj) and
add the new object in the shape object. This new object could contain a pointer
to the x and y values. It should take care of the performance lost without
adding the compilation option. However, I'm not sure how easy and how clean it
is to do that.

Any other idea?

Stephen Woodbridge: I'm pretty sure the compiler takes care of this issue.
Others may confirm or contradict that.

comment:5 Changed 17 years ago by jlacroix

Milestone: 4.6 release
Status: newassigned
Steve, since I did not receive any response, I will integrate the compile
option. However, if we want, we may change that later.

Aiming 4.6

comment:6 Changed 17 years ago by sdlime

Sounds ok I guess. (I was gone on vacation last week which is why you didn't 
hear from me...)

comment:7 Changed 17 years ago by jlacroix

Resolution: fixed
Status: assignedclosed
I commited my changes to remove the Z and M parameters in th pointObj by
default. Most of the work was pretty straight forward, but I did not test with
swig mapscript nor windows. The new configure flag is --enable-shape-z-m and the
define is USE_SHAPE_Z_M.

This gives around 7 to 10% performance gain in the overall.

Marking as FIXED

comment:8 Changed 17 years ago by sdlime

Thanks Julien, that's a significant enough gain to warrant the change then...


comment:9 Changed 17 years ago by sgillies@…

I've made a few changes to mapscript and the Python tests pass without the 
--enable-shape-z-m option.

I haven't run the tests with Z and M enabled.  Making this a compile option 
effectively doubles the amount of testing I have to do now, did you realize?
It was enough to stay on top of one MapServer, but now we have two different
programs and I have to stay on top of two configurations and builds.  Sort of
a pain.

My last two cents is that the option and macro should be changed to
--enable-point-z-m and USE_POINT_Z_M because these attributes are possessed 
by pointObj, not shapeObj.

comment:10 Changed 17 years ago by sgillies@…

Tests pass with --enable-shape-z-m

comment:11 Changed 17 years ago by dmorissette

I agree it would make sense to change to --enable-point-z-m and USE_POINT_Z_M
Could you please make that change Julien?

comment:12 Changed 17 years ago by jlacroix

Resolution: fixed
Status: closedreopened
I'll change shape-z-m to point-z-m

comment:13 Changed 17 years ago by jlacroix

Resolution: fixed
Status: reopenedclosed
I commited my changes to use USE_POINT_Z_M and --enable-point-z-m.

Marking as FIXED

comment:14 Changed 17 years ago by fsimon@…

Hi folks,
I will check the code for maporaclespatial.c.
A user report that using the last version of cvs the points from Oracle 
Spatial don't appear in Map.

comment:15 Changed 17 years ago by fsimon@…

Cc: fsimon@… added
Hi folks,
    I did the tests with Oracle Spatial connection, but the point only appear
with --enable flag. When I don't use this flag or user the --disable the points
(points, polygons, and lines) don't appear.
    After this I checked the maporaclespatial.c and I didn't fine problem with
the last changes. In my tests is only wotk with --enable-point-z-m
    Is it working with another connections (shape, PostGis)? Any tests?

comment:16 Changed 17 years ago by jlacroix

Shape, Tab, OVF work. I guess PostGIS works too since there's multiple user of
it.  I have few oracle data here. I will make some test.

comment:17 Changed 17 years ago by fsimon@…

    Problem solved.
    The SDOPointObj "need" the z value. It's a internal point Object for Oracle
(only used by Oracle). So, I remove the ifdef for this typedef and worked
without --enable flag. 
    I will do more tests to check it, but I belive that it's finished. 
    I saw in the code that only Oracle Spatail read and set the real z values
for points, it's right? PostGis, OGR, and Proj set the value to 0, right?

comment:18 Changed 17 years ago by jmckenna@…

Cc: jmckenna@… added
i'm setting up to test this issue with Oracle for Julien (although i don't have
the skill to go as deep as you guys).

comment:19 Changed 17 years ago by dmorissette

Jeff, Fernando has found and fixed the issue. There is probably no need to
duplicate the testing effort.

comment:20 Changed 17 years ago by jmckenna@…

sorry, i should have added myself to this bug earlier

comment:21 Changed 17 years ago by fsimon@…

Updated the new version of maporaclespatial.c
Note: See TracTickets for help on using tickets.