GNOME Bugzilla – Bug 768890
Add per map-source number of libsoup threads
Last modified: 2016-08-05 11:50:52 UTC
Because of Mapnik license, we currently limit the number of parallel libsoup threads which load tiles to 2. This number however depends on the map source licensing and if some map sources allow more threads, we should use this opportunity to load maps faster. The number of libsoup threads should be configurable on a per map-source basis.
Created attachment 332673 [details] [review] Ignore some files Ignore some autotools files and Emacs backup files.
Created attachment 332674 [details] [review] NetworkTileSrc: Make max-conns settable Make it possible to set the maximum allowed simultaneous connections on the underlying soup session. This makes it possible for NetworkTileSources that allow more simultanoeus connections than 2 to take advantage of that, making the map viewing experience much nicer.
This is the first C/GObject code I write, so I might very well have messed something up. It seems to work fine for me when testing against the mapbox tileset we use in Maps and for the demos in the repo though.
If you feel like testing this you can apply the patch above to Champlain and then this patch: http://paste.fedoraproject.org/400989/70266036/ to gnome-maps master.
Thanks Mattias, looks good, just the docstring in champlain_network_tile_source_set_max_conns() * @max_conns: TRUE when the tile source should be max_conns; FALSE otherwise should be corrected. Two questions: 1. Why do you set also the "max-conns" property? Isn't it sufficient to set "max-conns-per-host"? 2. Could you add the mapbox tile source to ChamplainMapSourceFactory if it's available for general use?
(In reply to Jiri Techet from comment #5) > Thanks Mattias, looks good, just the docstring in > champlain_network_tile_source_set_max_conns() > > * @max_conns: TRUE when the tile source should be max_conns; FALSE otherwise > > should be corrected. Will fix! > Two questions: > > 1. Why do you set also the "max-conns" property? Isn't it sufficient to set > "max-conns-per-host"? "max-conns" has a default max value of 10, if we set "max-conns-per-host" to something like 16 or 25 we'll still be capped at 10. > 2. Could you add the mapbox tile source to ChamplainMapSourceFactory if it's > available for general use? It's not for general use unfortunately. However, we could probably add a source somehow that asks you to add a key maybe? In general I'm not too fond of trying to add all tile providers inside champlain, my experience tells me that providers often come and go, change their ToS or are just set up by yourself.
Created attachment 332742 [details] [review] NetworkTileSrc: Make max-conns settable Make it possible to set the maximum allowed simultaneous connections on the underlying soup session. This makes it possible for NetworkTileSources that allow more simultanoeus connections than 2 to take advantage of that, making the map viewing experience much nicer.
Created attachment 332747 [details] [review] NetworkTileSrc: Make max-conns settable Make it possible to set the maximum allowed simultaneous connections on the underlying soup session. This makes it possible for NetworkTileSources that allow more simultanoeus connections than 2 to take advantage of that, making the map viewing experience much nicer. Version 3: Add reminder to developer to verify how many simultaneous connections the tile provider allows.
(In reply to Mattias Bengtsson from comment #6) > (In reply to Jiri Techet from comment #5) > > Thanks Mattias, looks good, just the docstring in > > champlain_network_tile_source_set_max_conns() > > > > * @max_conns: TRUE when the tile source should be max_conns; FALSE otherwise > > > > should be corrected. > > Will fix! > > > Two questions: > > > > 1. Why do you set also the "max-conns" property? Isn't it sufficient to set > > "max-conns-per-host"? > > "max-conns" has a default max value of 10, if we set "max-conns-per-host" to > something like 16 or 25 we'll still be capped at 10. OK. > > > 2. Could you add the mapbox tile source to ChamplainMapSourceFactory if it's > > available for general use? > > It's not for general use unfortunately. However, we could probably add a > source somehow that asks you to add a key maybe? Nah, probably not, I thought it was something for general use and not something that requires a user key. > > In general I'm not too fond of trying to add all tile providers inside > champlain, my experience tells me that providers often come and go, change > their ToS or are just set up by yourself. Agree but for newcomers it's nice to have a few map sources so developers can start using libchamplain immediately without any additional setup. But mapbox apparently isn't a good candidate.
I noticed a few more rather nitpicky things: - "max-conns-per-host", 2, /* This is as required by OSM */ - NULL); + "max-conns-per-host", 2, /* This is as required by OSM */ + "max-conns", 2, /* This is as required by OSM */ Just one of the comments about OSM requirements is enough here I think. + NULL); + + g_object_bind_property (G_OBJECT (tile_source), "max-conns", + G_OBJECT (priv->soup_session), "max-conns", + G_BINDING_DEFAULT); + g_object_bind_property (G_OBJECT (tile_source), "max-conns", + G_OBJECT (priv->soup_session), "max-conns-per-host", + G_BINDING_DEFAULT); Wouldn't it be more straightforward to set the properties of soup session in champlain_network_tile_source_set_max_conns() and drop the calls of g_object_bind_property() here? +gint +champlain_network_tile_source_get_max_conns (ChamplainNetworkTileSource *tile_source) +{ + g_return_val_if_fail (CHAMPLAIN_IS_NETWORK_TILE_SOURCE (tile_source), FALSE); Better return 0 instead of FALSE here.
(In reply to Jiri Techet from comment #9) > > In general I'm not too fond of trying to add all tile providers inside > > champlain, my experience tells me that providers often come and go, change > > their ToS or are just set up by yourself. > > Agree but for newcomers it's nice to have a few map sources so developers > can start using libchamplain immediately without any additional setup. But > mapbox apparently isn't a good candidate. Yeah, makes sense I guess. :)
(In reply to Jiri Techet from comment #10) > I noticed a few more rather nitpicky things: > > - "max-conns-per-host", 2, /* This is as required by OSM */ > - NULL); > + "max-conns-per-host", 2, /* This is as required by OSM */ > + "max-conns", 2, /* This is as required by OSM */ > > Just one of the comments about OSM requirements is enough here I think. True, will fix! > + NULL); > + > + g_object_bind_property (G_OBJECT (tile_source), "max-conns", > + G_OBJECT (priv->soup_session), "max-conns", > + G_BINDING_DEFAULT); > + g_object_bind_property (G_OBJECT (tile_source), "max-conns", > + G_OBJECT (priv->soup_session), "max-conns-per-host", > + G_BINDING_DEFAULT); > > Wouldn't it be more straightforward to set the properties of soup session in > champlain_network_tile_source_set_max_conns() and drop the calls of > g_object_bind_property() here? Indeed! I was thinking I had to something like that for when people used _set_property(…) directly but since that will just call _set_max_conns(…) anyways that's better. Will fix! > +gint > +champlain_network_tile_source_get_max_conns (ChamplainNetworkTileSource > *tile_source) > +{ > + g_return_val_if_fail (CHAMPLAIN_IS_NETWORK_TILE_SOURCE (tile_source), > FALSE); > > Better return 0 instead of FALSE here. Yep! An oversight from copying boilerplate. Will fix.
(In reply to Jiri Techet from comment #10) > + NULL); > + > + g_object_bind_property (G_OBJECT (tile_source), "max-conns", > + G_OBJECT (priv->soup_session), "max-conns", > + G_BINDING_DEFAULT); > + g_object_bind_property (G_OBJECT (tile_source), "max-conns", > + G_OBJECT (priv->soup_session), "max-conns-per-host", > + G_BINDING_DEFAULT); > > Wouldn't it be more straightforward to set the properties of soup session in > champlain_network_tile_source_set_max_conns() and drop the calls of > g_object_bind_property() here? Actually, would it make sense to not store the value in this class at all then and just forward to soup_session->max_conns_per_host? Not that it matters much, but no need to store the value one extra time is my thinking.
> Actually, would it make sense to not store the value in this class at all > then and just forward to soup_session->max_conns_per_host? > Not that it matters much, but no need to store the value one extra time is > my thinking. It might be possible but we'd have to make sure the code doesn't crash when priv->soup_session == NULL (and there might be some strange situations like that when the object is disposing and something calls the getter). I'd say keeping the variable is more standard and we don't have to think much about when soup_session is NULL.
Created attachment 332754 [details] [review] NetworkTileSrc: Make max-conns settable Make it possible to set the maximum allowed simultaneous connections on the underlying soup session. This makes it possible for NetworkTileSources that allow more simultanoeus connections than 2 to take advantage of that, making the map viewing experience much nicer. Version 4: - Remove extra osm tileset comment - Set max_conns/max_conns_per_host in set_max_conns - Return 0 instead of false on get_max_conns
Created attachment 332755 [details] [review] NetworkTileSrc: Define magic number Use a #define for the maximum connection default of 2 and explain why 2 is the default a little more thoroughly.
(In reply to Jiri Techet from comment #14) > > Actually, would it make sense to not store the value in this class at all > > then and just forward to soup_session->max_conns_per_host? > > Not that it matters much, but no need to store the value one extra time is > > my thinking. > > It might be possible but we'd have to make sure the code doesn't crash when > priv->soup_session == NULL (and there might be some strange situations like > that when the object is disposing and something calls the getter). I'd say > keeping the variable is more standard and we don't have to think much about > when soup_session is NULL. Yep, agree!
Review of attachment 332754 [details] [review]: ::: champlain/champlain-network-tile-source.c @@ +562,3 @@ + "max-conns-per-host", max_conns, + "max-conns", max_conns, + NULL); Is this safe, or do I need to make a null check on soup_session?
(In reply to Mattias Bengtsson from comment #18) > Review of attachment 332754 [details] [review] [review]: > > ::: champlain/champlain-network-tile-source.c > @@ +562,3 @@ > + "max-conns-per-host", max_conns, > + "max-conns", max_conns, > + NULL); > > Is this safe, or do I need to make a null check on soup_session? Maybe better to add an if to be sure.
Created attachment 332756 [details] [review] NetworkTileSrc: Make max-conns settable Make it possible to set the maximum allowed simultaneous connections on the underlying soup session. This makes it possible for NetworkTileSources that allow more simultanoeus connections than 2 to take advantage of that, making the map viewing experience much nicer. Version 5: - Add soup_session null check
Committed, thanks!
\o/