Using thread local variables instead of the global ones (MS-RFC-15)

[Stephen Woodbridge]

Tamás,

An excellent, well written and clear RFC. I think you have done a great 
job breaking this large effort into well defined and prioritized 
tactics. Thank you for tackling this important issue for mapserver.

-Steve W.

Szekeres Tamás wrote:
> 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,