maplibre / martin

Blazing fast and lightweight PostGIS, MBtiles and PMtiles tile server, tile generation, and mbtiles tooling.
https://martin.maplibre.org
Apache License 2.0
2.33k stars 216 forks source link

feat!: `DashMap` based catalog #1585

Open CommanderStorm opened 1 week ago

CommanderStorm commented 1 week ago

This PR changes the internal datastructure to be Dashmap<..> as suggested in https://github.com/maplibre/martin/pull/1179#pullrequestreview-2149936800 I thought that the data/configuration/.. split might be more managble in terms of reviwing and shipping this.

Sadly two changes did cascade a bit:

I had some free time and this seemed like a nice issue to progress on, I hope you (@sharkAndshark) don't mind me stealing this part ^^

CommanderStorm commented 1 week ago

Here is a benchmark of the versions "requesting" tiles (i.e. without the network/file io overhead)

branch features image
master - image
dashmap-based-catalog inline image
dashmap-based-catalog rayon image
dashmap-based-catalog rayon
inline
image

=> it is a significant performance drop, but 60ns to enable updating is likely an okay tradeoff. What do you people think?

sharkAndshark commented 1 week ago

I'm really happy to see you pick this up again. It's a looooooong requested feature. And Martin needs more contributor. Go ahead! : D @CommanderStorm

sharkAndshark commented 1 week ago

Consider making the use of dashmap and /refresh API optional (with a default)? @nyurik This would help users like me who don't mind rebooting Martin to reflect database updates.

CommanderStorm commented 1 week ago

Consider making dashmap optional

No, splitting the implementation on one of the core datastructures is a great way to get bugs and maintainability issues. The 60ns performance drop should not matter to you (remember that 60ns = 0.000060ms)

/refresh is not included in this PR, but will be disablable in the config as far as I read the spec

sharkAndshark commented 1 week ago

60ns means nothing to me. I'm totally Ok the performance part. It's just personal flavor . So yes let's use dashmap and only make /refresh optional(in the next PRs):D