feat!: DashMap based catalog
#1585
Conversation
|
Here is a benchmark of the versions "requesting" tiles (i.e. without the network/file io overhead)
=> it is a significant performance drop, but 60ns to enable updating is likely an okay tradeoff. What do you people think? |
|
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 |
|
Consider making the use of |
No, splitting the implementation on one of the core datastructures is a great way to get bugs and maintainability issues.
|
|
60ns means nothing to me. I'm totally Ok the performance part. |
There was a problem hiding this comment.
great work!! Seriously impressive :)
A few things:
- could you explain a bit why you need boxed source? Can we work around it?
- please move the sorting output into a separate PR - this way test changes do not get in the way of the review
- this also goes for anything unrelated to this pr - switching to dashmap is a complex PR - so the fewer changes we have in it, the easier it is to spot issues
plus a few minor inlines
martin/src/bin/martin-cp.rs
Outdated
| @@ -392,7 +392,7 @@ fn parse_encoding(encoding: &str) -> MartinCpResult<AcceptEncoding> { | |||
| async fn init_schema( | |||
| mbt: &Mbtiles, | |||
| conn: &mut SqliteConnection, | |||
| sources: &[&dyn Source], | |||
| sources: &[Box<dyn Source>], | |||
There was a problem hiding this comment.
I'm still not certain why you need a boxed type here - what caused it to change? Also, you might want to use TileInfoSource for consistency
martin/src/sprites/mod.rs
Outdated
| @@ -198,18 +195,18 @@ async fn parse_sprite( | |||
| } | |||
|
|
|||
| pub async fn get_spritesheet( | |||
| sources: impl Iterator<Item = &SpriteSource>, | |||
| paths: impl Iterator<Item = &PathBuf>, | |||
There was a problem hiding this comment.
is this relevant to this PR? I am usually ok to mix in a bunch of unrelated changes into smaller / linter / cleanup PRs, but in case of a the complex PRs that already do a lot of fundamental system changes, it is better to move unrelated stuff to a separate PR
| pub fn get_source(&self, id: &str) -> actix_web::Result<TileInfoSource> { | ||
| Ok(self | ||
| .0 | ||
| .get(id) | ||
| .ok_or_else(|| ErrorNotFound(format!("Source {id} does not exist")))? | ||
| .as_ref()) | ||
| .value() | ||
| .clone()) | ||
| } |
There was a problem hiding this comment.
Lets provide the rationale why the Box change was done:
The change to the boxed variant comes from here and cascades pretty far sadly.
Because accessing a value returns Ref<..> => a temporary variable
This PR pulls the changes to `Box<dyn Source>` vs `TileInfoSource` from - #1585
CommanderStorm
force-pushed
the
dashmap-based-catalog
branch
from
f941c65 to
935f30c
Compare
This PR changes the internal datastructure to be
Dashmap<..>as suggested in #1179 (review)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:
&dyn Source->Box<dyn Source>DashMapdoes not guarantee sorting of keys => I need to manually do soI 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 ^^