[mapguide-internals] Initial review on RFC90 code

[UV]

Hi Trevor,

thanks for the reply. I think we should be able to find a time on monday 
later afternoon CET.
I lost the thursday to OS issues so I am a bit behind schedule but I 
gonna try hard to finish this off this month.
I send my skype address to you directly.

cheers,
UV
On 6/11/2010 18:22, Trevor Wekel wrote:
> Hi UV,
>
> A couple of comments below.  We can schedule time for a chat session.  When are you available?
>
> GetTileCoords() move to MgMap
> - Other use cases include programmatic calculation of tile location by external applications.
thats fine so it goes.
> DetectLockFile() move to MgTileCache
> - The locking mechanism (whatever it happens to be) is for tiles and should be encapsulated/hidden accordingly.
> - I will also look into ACE::Conditions.  It's always better to have ACE do the work for us...
I implemented a hashmap of ACE:conditions for the tile grid as a quick 
shot and we had no issues yet.
But I didnt delve too deep in the depth of the underlying OS 
implementation, so in terms of resources there is likely to be room for 
optimization in this algorithm. Well worth a discussion.
> Regards,
> Trevor
>
> -----Original Message-----
> From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of UV
> Sent: November 5, 2010 4:36 AM
> To: MapGuide Internals Mail List
> Subject: Re: [mapguide-internals] Initial review on RFC90 code
>
> 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
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals