Using thread local variables instead of the global ones
(MS-RFC-15)
Szekeres Tamás
szekeres.tamas at FREEMAIL.HU
Mon May 1 18:27:35 EDT 2006
Developers,
I was thinking a lot how to move forward with this issue. Since we are
tending to do something i have added a new RFC
http://mapserver.gis.umn.edu/development/rfc/ms-rfc-15/
also added a bug to collect the developer and user comments and update the
RFC accordingly.
Feel free to send your comments, modification suggestions, be a co-author
etc. This RFC is a bit special since wide audience and contribution is
needed from the maintainers of the different portions of the code.
Thanks,
Tamas Szekeres
-----Original Message-----
From: UMN MapServer Developers List [mailto:MAPSERVER-DEV at LISTS.UMN.EDU] On
Behalf Of Frank Warmerdam
Sent: Friday, April 28, 2006 4:25 PM
To: MAPSERVER-DEV at LISTS.UMN.EDU
Subject: Re: [UMN_MAPSERVER-DEV] Using thread local variables instead of the
global ones
Szekeres Tamás wrote:
> At a first sight i have found *only* the following problems
> in the core mapserver code. Some of the variables contain
> invariant data and therefore not to be guarded.
Tamas,
I'll try and comment on a few of these.
> \mapserver\epplib.c(47):static int REVERSE; /* set to 1 on
> bigendian machines */
EPP support is very narrowly used, and I don't think we need worry
about it.
> \mapserver\mapcpl.c(57):static char
> szStaticResult[MS_PATH_BUF_SIZE];
In fact this code carried over from GDAL was later remodelled in
GDAL. It looks like msGetBasename() is only used in a few places
and we could remodel those to avoid using a static buffer.
> \mapserver\maperror.c(110):static char
> *ms_errorCodes[MS_NUMERRORCODES] = {"",
There is no problem with the error codes being static as far as
I know. I think perhaps they should be declared static const char
to indicate they are unchanging.
> \mapserver\maperror.c(154): static errorObj ms_error =
> {MS_NOERR, "", "", NULL};
> \mapserver\maperror.c(169):static te_info_t *error_list = NULL;
In fact, when USE_THREAD is enabled, the static errorObj isn't
used. Instead a per-thread error object is maintained and
selection is managed based on the threadid. This was a crude
implementation of thread local data I did when first adding
multi-thread support. I believe this area is safe though it could
stand some remodelling if we had a better approach to thread local
data.
> \mapserver\maperror.c(552): static char version[1024];
More static stuff.
> \mapserver\maperror.c(651): static char nonblocking_set = 0;
Harmless I believe.
> \mapserver\mapfile.c(178):static char *msUnits[7]={"INCHES",
> "FEET", "MILES", "METERS", "KILOMETERS", "DD", "PIXELS"};
> \mapserver\mapfile.c(179):static char
> *msLayerTypes[8]={"POINT", "LINE", "POLYGON", "RASTER",
> "ANNOTATION", "QUERY", "CIRCLE", "TILEINDEX"};
> \mapserver\mapfile.c(182):static char
> *msBitmapFontSizes[5]={"TINY", "SMALL", "MEDIUM", "LARGE",
> "GIANT"};
> \mapserver\mapfile.c(183):static char
> *msQueryMapStyles[4]={"NORMAL", "HILITE", "SELECTED",
> "INVERTED"};
> \mapserver\mapfile.c(184):static char *msStatus[5]={"OFF",
> "ON", "DEFAULT", "EMBED"};
> \mapserver\mapfile.c(186):static char
> *msTrueFalse[2]={"FALSE", "TRUE"};
> \mapserver\mapfile.c(188):static char
> *msJoinType[2]={"ONE-TO-ONE", "ONE-TO-MANY"};
>
> \mapserver\mapgd.c(236):static unsigned char PNGsig[8] =
> {137, 80, 78, 71, 13, 10, 26, 10}; /* 89 50 4E 47 0D 0A 1A
> 0A hex */
> \mapserver\mapgd.c(237):static unsigned char JPEGsig[3] =
> {255, 216, 255}; /* FF D8 FF hex */
The above are all const static data, and no problem.
> \mapserver\mapgd.c(908): static gdPoint points[38];
I think this one should not be static.
> \mapserver\mapgd.c(2425): static double last_style_size;
> \mapserver\mapgd.c(2716): static int styleIndex, styleVis;
> \mapserver\mapgd.c(2717): static double styleSize=0,
> styleCoef=0, last_style_size=-1;
> \mapserver\mapgd.c(2718): static int last_style_c=-1,
> last_style_stylelength=-1, last_styleVis=0;
I think the above are not needing to be static, but it is a
bit hard to tell without more careful investigation.
> \mapserver\mapgdal.c(141):static int bGDALInitialized = 0;
This only changes once on initialization. As long as msSetup()
is called to initialize things I think we are ok.
> \mapserver\mapgeos.cpp(98):GeometryFactory *gf=NULL;
Likewise, this only changes once on initialization.
> \mapserver\maphttp.c(140):static int gbCurlInitialized =
> MS_FALSE;
Ditto
> \mapserver\mapimagemap.c(141):static char *layerlist=NULL;
> \mapserver\mapimagemap.c(142):static int layersize=0;
> \mapserver\mapimagemap.c(143):static pString imgStr,
> layerStr={ &layerlist, &layersize, 0 };
> \mapserver\mapimagemap.c(146):static const char
> *polyHrefFmt, *polyMOverFmt, *polyMOutFmt;
> \mapserver\mapimagemap.c(147):static const char
> *symbolHrefFmt, *symbolMOverFmt, *symbolMOutFmt;
> \mapserver\mapimagemap.c(148):static const char *mapName;
> \mapserver\mapimagemap.c(150):static int suppressEmpty=0;
> \mapserver\mapimagemap.c(229):static int lastcolor=-1;
> \mapserver\mapimagemap.c(273):static char* lname;
> \mapserver\mapimagemap.c(274):static int dxf;
Whew! Well, lets just say that imagemap support (not widely
used at all) is not threadsafe.
> \mapserver\mapio.c(67):static int is_msIO_initialized =
> MS_FALSE;
> \mapserver\mapio.c(69):static msIOContext default_stdin_context;
> \mapserver\mapio.c(70):static msIOContext
> default_stdout_context;
> \mapserver\mapio.c(71):static msIOContext
> default_stderr_context;
> \mapserver\mapio.c(73):static msIOContext current_stdin_context;
> \mapserver\mapio.c(74):static msIOContext
> current_stdout_context;
> \mapserver\mapio.c(75):static msIOContext
> current_stderr_context;
Currently there is only one process wide set of "io handlers" for
io. This will almost certainly need to change at some point to be
thread local in some fashion. I hope to address this when I work
on the redirectable OWS services accessable from mapscript this spring.
As things stand this is fine as long as only stdout output is being
used, or if other mechanisms (via mapscript) are used.
> \mapserver\maplexer.c(220):static YY_BUFFER_STATE
> yy_current_buffer = 0;
> \mapserver\maplexer.c(230):static char yy_hold_char;
> \mapserver\maplexer.c(232):static int yy_n_chars; /* number
> of characters read into yy_ch_buf */
> \mapserver\maplexer.c(238):static char *yy_c_buf_p = (char *) 0;
> \mapserver\maplexer.c(239):static int yy_init = 1; /*
> whether we need to initialize */
> \mapserver\maplexer.c(240):static int yy_start = 0; /* start
> state number */
> \mapserver\maplexer.c(306):static yyconst short int
> yy_accept[2406] =
> \mapserver\maplexer.c(575):static yyconst int yy_ec[256] =
> \mapserver\maplexer.c(607):static yyconst int yy_meta[52] =
> \mapserver\maplexer.c(617):static yyconst short int
> yy_base[2420] =
> \mapserver\maplexer.c(887):static yyconst short int
> yy_def[2420] =
> \mapserver\maplexer.c(1157):static yyconst short int
> yy_nxt[2740] =
> \mapserver\maplexer.c(1462):static yyconst short int
> yy_chk[2740] =
> \mapserver\maplexer.c(1767):static yy_state_type
> yy_last_accepting_state;
> \mapserver\maplexer.c(1768):static char *yy_last_accepting_cpos;
> \mapserver\maplexer.c(1866):static int yy_start_stack_ptr = 0;
> \mapserver\maplexer.c(1867):static int yy_start_stack_depth = 0;
> \mapserver\maplexer.c(1868):static int *yy_start_stack = 0;
I believe Steve suggested we can address this with newer
versions of bison and flex invoked appropriately.
> \mapserver\mapmygis.c(245):static int gBYTE_ORDER = 0;
I think this could be moved into a per-layer structure, but
the "mygis" code isn't well maintained.
> \mapserver\mapogcsos.c(1974): static char *request=NULL,
> *service=NULL;
I'm not sure if this needs to be static. Assefa, could you
review?
> \mapserver\mapogr.cpp(840):static int bOGRDriversRegistered
> = MS_FALSE;
Usual startup initialization.
> \mapserver\mapows.c(1647): static char epsgCode[20] ="";
Whew, this is used in quite a few places. It could be made
thread-local data or perhaps the code depending on it could
be remodelled.
> \mapserver\mapparser.c(282):static const unsigned char
> yytranslate[] =
> \mapserver\mapparser.c(317):static const unsigned char
> yyprhs[] =
> \mapserver\mapparser.c(328):static const yysigned_char yyrhs[] =
> \mapserver\mapparser.c(354):static const unsigned short
> yyrline[] =
> \mapserver\mapparser.c(368):static const char *const yytname[] =
> \mapserver\mapparser.c(381):static const unsigned short
> yytoknum[] =
> \mapserver\mapparser.c(391):static const unsigned char yyr1[] =
> \mapserver\mapparser.c(402):static const unsigned char yyr2[] =
> \mapserver\mapparser.c(415):static const unsigned char
> yydefact[] =
> \mapserver\mapparser.c(431):static const yysigned_char
> yydefgoto[] =
> \mapserver\mapparser.c(439):static const short yypact[] =
> \mapserver\mapparser.c(455):static const yysigned_char
> yypgoto[] =
> \mapserver\mapparser.c(465):static const unsigned char
> yytable[] =
> \mapserver\mapparser.c(491):static const yysigned_char
> yycheck[] =
> \mapserver\mapparser.c(519):static const unsigned char
> yystos[] =
As per lexer, use new bison with right options.
> \mapserver\mappluginlayer.c(46):static VTFactoryObj
> gVirtualTableFactory = {MS_MAXLAYERS, 0, {NULL}};
Intended to be global, and properly protected by lock.
> \mapserver\mappool.c(206):static int connectionCount = 0;
> \mapserver\mappool.c(207):static int connectionMax = 0;
> \mapserver\mappool.c(208):static connectionObj *connections
> = NULL;
Intended to be global and properly protected by lock.
> \mapserver\mapproject.c(889):static char *ms_proj_lib = NULL;
> \mapserver\mapproject.c(890):static char *last_filename = NULL;
> \mapserver\mapproject.c(918): static int finder_installed
> = 0;
Hmm, this might need rework.
> \mapserver\mapscale.c(68):static char *unitText[5]={"in",
> "ft", "mi", "m", "km"};
> \mapserver\mapscale.c(69):double inchesPerUnit[6]={1, 12,
> 63360.0, 39.3701, 39370.1, 4374754};
const static data.
> \mapserver\mapsde.c(216):static int lcacheCount = 0;
> \mapserver\mapsde.c(217):static int lcacheMax = 0;
> \mapserver\mapsde.c(218):static layerId *lcache = NULL;
Protected by SDE lock.
> \mapserver\mapshape.c(70):static int bBigEndian;
this is a one time initialization. No danger.
> \mapserver\mapsvg.c(164): static char epsgCode[20] ="";
> \mapserver\mapsvg.c(165): static char *value;
I'm not too sure about this one.
> \mapserver\mapswf.c(97):static char gszFilename[128];
> \mapserver\mapswf.c(98):static char gszAction[256];
> \mapserver\mapswf.c(99):static char gszTmp[256];
Or this.
> \mapserver\mapsymbol.c(151):static char
> *msCapsJoinsCorners[7]={"NONE", "BEVEL", "BUTT", "MITER",
> "ROUND", "SQUARE", "TRIANGLE"};
const static.
> \mapserver\mapthread.c(196):static int thread_debug = 0;
should be const static.
> \mapserver\mapthread.c(198):static char *lock_names[] =
const static.
> \mapserver\mapthread.c(213):static int mutexes_initialized = 0;
> \mapserver\mapthread.c(214):static pthread_mutex_t
> mutex_locks[TLOCK_MAX];
one time init.
> \mapserver\mapthread.c(223): static pthread_mutex_t
> core_lock = PTHREAD_MUTEX_INITIALIZER;
one time init.
> \mapserver\mapthread.c(294):static int mutexes_initialized = 0;
> \mapserver\mapthread.c(295):static HANDLE
> mutex_locks[TLOCK_MAX];
> \mapserver\mapthread.c(305): static HANDLE core_lock = NULL;
one time init.
> \mapserver\maputil.c(1068):static int tmpCount = 0;
> \mapserver\maputil.c(1069):static char *ForcedTmpBase = NULL;
These are generally only set once. Limitations should be made clear.
> \mapserver\mapwms.c(249):static char *wms_exception_format=NULL;
> \mapserver\mapwms.c(2779): static char *request=NULL,
> *service=NULL, *format=NULL;
not too sure.
> \mapserver\md5c.c(58):static unsigned char PADDING[64] = {
>
> \mapserver\strptime.c(43):static const char *abb_weekdays[] = {
> \mapserver\strptime.c(54):static const char *full_weekdays[] = {
> \mapserver\strptime.c(65):static const char *abb_month[] = {
> \mapserver\strptime.c(81):static const char *full_month[] = {
> \mapserver\strptime.c(97):static const char *ampm[] = {
static const.
> \mapserver\gdft\gdkanji.c(103): static int whatcode;
> \mapserver\gdft\gdkanji.c(403): static unsigned char
> tmp[BUFSIZ];
> \mapserver\gdft\gdkanji.c(489): static unsigned char
> tmp_dest[BUFSIZ];
I don't know. I didn't realize this directory existed.
> \mapserver\gdft\gdttf.c(712): static gdCache_head_t
> *tweenColorCache=NULL; /****** set up tweenColorCache on
> first call ************/
> \mapserver\gdft\gdttf.c(852): static gdCache_head_t
> *fontCache=NULL; /****** initialize font engine on first
> call ************/
> \mapserver\gdft\gdttf.c(853): static TT_Engine engine;
> \mapserver\gdft\jisx0208.h(7):static unsigned short
> UnicodeTbl[][94] = {
Ditto.
I don't really think we are in bad shape.
Best regards,
--
---------------------------------------+------------------------------------
--
I set the clouds in motion - turn up | Frank Warmerdam,
warmerdam at pobox.com
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush | President OSGF, http://osgeo.org
--
No virus found in this incoming message.
Checked by AVG Free Edition.
Version: 7.1.385 / Virus Database: 268.5.1/327 - Release Date: 2006.04.28.
--
No virus found in this outgoing message.
Checked by AVG Free Edition.
Version: 7.1.385 / Virus Database: 268.5.1/327 - Release Date: 2006.04.28.
More information about the mapserver-dev
mailing list