Opened 15 years ago

Last modified 12 years ago

#955 new enhancement

Performance bottlenecks with parser use in msEvalExpression

Reported by: dmorissette Owned by: sdlime
Priority: high Milestone: FUTURE
Component: MapServer C Library Version: 4.3
Severity: minor Keywords:
Cc: unicoletti@…

Description

Bug 339 fixed some thread-safety issues but left some potential performance
bottlenecks. This is enhancement bug is to discuss and address them. Here are
the relevant comments from bug 339:

------- Additional Comment -- 10 From Sean Gillies  2004-10-10 11:04 -------

Recent progress on thread safety:

I've fixed trouble at three spots

- parsing expression strings outside of msLoadMap
- evaluating mapserver logical expressions
- loading symbol set files outside of msLoadMap

while leaving Frank's mapthread code unchanged.  It's simple and works, no
need to change it.

Issues of thread safety can now be divided into two separate application
phases: a "loading" phase and an "execution" phase, which is everything else.

During the loading phase, we acquire a parser lock immediately and release
it only after the entire mapfile has been loaded.  Within this phase we
will use parsing functions with no locking of their own, particularly
loadExpressionString, and loadSymbolSet.

Outside of the loading phase, we must use locking versions of these functions.
msLoadExpressionString and msLoadSymbolSet are wrappers for the previous
functions.  I've also added parser locking to msEvalExpression and msEvalContext,
and have changed all execution-phase code to use these parser locking
functions.

My tests pass, the code is working fine in my multithreaded Zope app, so
I'm going to call it "FIXED".

We are now safer, but still have a performance bottleneck.  The one 
parser is shared between threads in mapfile loading and map rendering (or
"execution") phases.  In the first case a lock is held relatively long,
while in the second case it's acquired and released (inside msEvalExpression)
quickly with every call to msLayerNextShape.  I believe the tendency would be
for map rendering to be upstaged by mapfile loading when activity is high.

I see at least three solutions that we may want to consider as a new 
tracker issue for 4.4: 

1) implement a second global parser for use in msEvalExpression

and/or

2) pull the parser locking out of msEvalExpression, and instead lock around
the loops which call msLayerNextShape.

or

3) rewrite msEvalExpression so that it doesn't need a parser for logical
expressions.



------- Additional Comment -- 11 From Frank Warmerdam 2004-10-12 08:47 -------

Sean Gillies wrote:
> 1) implement a second global parser for use in msEvalExpression

Sean,

My understanding (and Steve obviously knows more about this) is that the
mapfile parsing uses only the lexer while msEvalExpression() uses the
yacc/bison parser which in turn uses the same low level lexer as the
mapfile code uses.  It might be possible to build a seperate lexer instance
for the expressions.  
 
> and/or
> 
> 2) pull the parser locking out of msEvalExpression, and instead lock around
> the loops which call msLayerNextShape.

I don't think this would be all that wise. My take would be that 
mapfile parsing is pretty quick while in some cases processing shapes
can take a long time, much of which is not spent in the parser at all.
 
> or
> 
> 3) rewrite msEvalExpression so that it doesn't need a parser for logical
> expressions.
> 
> I'd love to get some feedback from folks who are interested in using
> mapserver in a multithreaded environment.

Well, my longer term suggestion has always been to look into whether we
can get a thread safe lexer in place, either by researching flex options
in greater depth or if necessary by rewriting the lexer by hand.  Lexers
aren't really that hard.

Change History (6)

comment:1 Changed 15 years ago by dmorissette

My 0.02$:

I think only #1 and #3 would be viable options for the future, with #3 preferred
of course if we can find a thread-safe parser. About #2, I would be worried that
if we do that we could forget to put locking around new code or MapScript
methods that may not be calling msEvalExpression() today but that may end up
calling it later down the road.

comment:2 Changed 15 years ago by sgillies@…

Agreed, #2 is no good.  Many potential problems there.

I'm actually quite happy with our present position.  I try to avoid heavy
use of mapserver logical expressions anyway.  A second lexer or rewrite
of the expression parser is low priority for me.

comment:3 Changed 15 years ago by sdlime

You'll need a parser for logical expressions regardless. If we go to XML
mapfiles with a different parser on that end then only bison/flex will be needed
for expressions and this will be an easier change. Flex can generate a
re-entrant lexer and bison can create a threadsafe parser, but that sort of a
switch is 5.x and is affected by other things. Mapfile.c contains the mapfile
parser, it's the lexer that's doubled up. Could have two of those buried in a
mapObj I suppose.

I agree with Dan that there are bigger fish to fry at the moment.

Steve

comment:4 Changed 15 years ago by dmorissette

Milestone: FUTURE
It seems that we all agree that this won't be addressed in 4.4.

I have created a new "FUTURE" target milestone to mark bugs that should be
revisited and possibly scheduled in a future release and I have set target
milestone FUTURE for this bug.

comment:5 Changed 13 years ago by sgillies@…

Owner: changed from sgillies@… to sdlime
reassigning.

comment:6 Changed 12 years ago by unicoletti

Cc: unicoletti@… added

Bookmark Notes on how to enable the reentrant feature (thread safe) of flex:

http://flex.sourceforge.net/manual/Reentrant.html#Reentrant

Note: See TracTickets for help on using tickets.