[mapguide-internals] Initial review on RFC90 code
Trevor Wekel
trevor_wekel at otxsystems.com
Sat Nov 6 13:22:38 EDT 2010
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.
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...
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
More information about the mapguide-internals
mailing list