[mapguide-internals] Initial review on RFC90 code

[UV]

Hi Trevor,

I finally managed to get back to this and are currently working to 
resolve some of the mentioned issues.
Thanks a lot for the comments.

However, some comments are not 100% clear to me or there is a config 
reason behind.
I think a direct chat session would be a most efficient way to sort out 
the remaining issues.
I am on CET now and have time for a few days between changing jobs.

Comments on each issue inline below.

On 27/04/2010 20:09, Trevor Wekel wrote:
> Hi UV,
>
> Here are some comments from my initial review of the RFC 90 code.  I will do a follow up review once the RFC has been updated and code changes have been made.
>
> Thank you,
> Trevor
>
>
> General Comments
> ----------------
> ----------------
>
> RFC 90 documentation in Trac is incomplete.  All serverconfig.ini parameters and new function signatures should be documented.  A general algorithm overview would also be good.
>
> Based on my initial review, the following items should be documented:
> - useMetaTiles and LockMethod serverconfig.ini parameters.
> - Behavioural modification to AGGRenderer::Save and AGGImageIO.cpp
> - New MgTileCache behaviour (see below, I believe some refactoring would be appropriate)
>
> File specific comments
> ----------------------
> ----------------------
>
> RenderingService.h
> ProxyRenderingService.h
> -----------------------
> - new RenderFromMetaTile signature commented out, should it be removed?
>
> TileService.h
> -------------
> - space change is not necessary.
>
> ConfigProperties.cpp
> ConfigProperties.h
> ------------------
> - TileServicePropertyUseMetaTiles, TileServicePropertyLockMethod should be doc'd in RFC.
> - New properties should be added to MgConfigProperties::sm_cviTileServiceProperties array
OK
> AGGRenderer.cpp
> ---------------
> - "META" image format could be defined in MgImageFormats as an INTERNAL_API
> - new ::SaveMethod uses format = "META".  I am not 100% clear on how this all fits together.  Some algorithm documetation in the RFC would be helpful.
> - Can assert be replaced by a throw (MgInvalidArgumentException)?
started and in progress.
> AggImageIO.cpp
> --------------
> - Please remove commented out code.
>
> Server.sln
> ----------
> - Why were "Static" configurations added?
by accident ;)
> ServerRenderingService.vcproj
> -----------------------------
> - Rendering Service should not depend on Tile service headers.
I know, but I needed these dependencies due  to the way the metatile is 
being passed around.  As mentioned above I would appreciate a brief 
discussion regarding a different approach.
> ServerRenderingService.cpp
> --------------------------
> - GetTileCoords()
> What purpose does tileColOffset serve?
> Could this method be moved into MgMap as an INTERNAL_API method which takes tileWidth and tileHeight as parameters?
This is to walk the subtiles over the metatile.  I did method extraction 
from the formula on line 173 in trunk to compute the coords of the 
subtiles. Maintaining locality of this use case for metatiling in 
renderingservice, I did not bother with moving it to the MgMap. Are 
there other use cases?

However, I can really use some help here. Currently the system always 
renders metatiles bigger than the actual map.
I could not figure out how the border of the map fits into the metatile 
to suppress metatiles along the borders.
I think it does not cost to much to render the additional empty borders 
but it seems superfluous. So if there is a reasonable way to cut this 
off we should talk about it.

- RenderTileFromMetaTile()
> Can asserts be replaced by MgInvalidArgumentException, MgOutOfRangeException?
> Should bsimp->GetLength() be tested for 0<  size<  2^31 before casting?
> Why is MyByteSource required?  The direct cast is fragile.
I am open for suggestions how to do it differently!
I tried to avoid touching Foundation classes, but those are not really 
made for this so I had to use a cast.
I was juggling quite a bit with parameters and types to find the least 
intrusive way to pass the metatile bitmap back from the RenderingService 
back to TileService.
As a non comitter those choices are guided by minimizing the sometimes 
very tedious merges!
> ServerTileService.cpp
> ---------------------
> - RFC documentation for sm_creationCutoffTime, sm_pollingInterval, sm_useMetaTiles, sm_lockMethod
OK coming up
> - sm_creationCutoffTime and sm_pollingInterval should use same time base.
I'd rather drop the polling Interval and use the ACE_Conditions .... 
specifying cutofftime as 120000 seems impractical.
> - MgServerTileService::MgServerTileService() : MgTileService()
> METAMAXDIM should be moved to configuration parameters MG_CONFIG_MAX_METATILE_DIM
> Error message should use MgConfigurationPropertyValueIsOutOfRange
> <sstream>  use could be dropped.
normal strings are typedef wstring STRING;
This is debug code which IMHO should not clutter the important logic, so 
I chose the shortest way to code such a concatenation of values
by using std::wstringstream. I can add a macro to FoundationDefs.h next 
to STRING if thats better.
Is there any other way to do a less verbose string concatenation?
(Sometimes I wonder how huge your screens must be considering the 
spacious coding style. ;-)
> - DetectLockFile()
> ------------------
> This functionality should be moved into MgTileCache and probably renamed to WaitForTileLock.
> Is a subsecond sleep really necessary?
As a performing tuning measure I figured it would have some impact to 
have more fine grained control.  Thinking about multicore systems > 4 
cores this could speed up things significantly for small tiles. But I am 
not insisting on this and personally more into the ACE::Conditions.
> ACE_OS::sleep (const ACE_Time_Value&tv) may simplify code
thats whats used on line 144
> - GetTile()
> -----------
> Since there is no signature change to the public API, APIVERSION 1.2 is not really required.
I added those comments to understand the sparsely documented code 
better. If its wrong I can remove it. Otherwise its documentation.
> - GetSingleTile()
> -----------------
> MgFoundBlockingLockFile must be added to resource string file(s) in MapGuideCommon/Resources
done
> - GetMetaTile()
> ---------------
> Locking methods should be documented in the RFC.
> Algorithm can be simplified by only generating a lock for the left top corner.  Meta tiling will be always on or always off.  For future work, each map will also be meta tiled or not.
> I believe there is too much special case locking logic coded here.  The locking logic should be moved into MgTileCache (LockTile(), UnlockTile(), WaitForTileLock()).
> All error messages must be defined in MapGuideCommon/Resources
> <sstream>  use could be dropped.
lets talk about this please.
> TileCache.h
> -----------
> - GetLockMap()
> Lock map structure should not be directly exposed using GetLockMap().  LockTile() and UnlockTile() methods should be added.  New methods should hide sm_lockMethod details from the rest of the code.
I gonna have a look at this refactoring.
> serverconfig.ini
> ----------------
> - Font names seem to have changed.  Please check editor.
OK
>
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internal