Implement file based cache

[swarnat]

We heavily improved the performance of this library by using an optional file based cache.

We decided against the usage of cache system, provided by got package, because there is no compatible cache provider. And data is big enough to be stored in file.

I marked this as WIP, because this it currently in test phase of our software. We also will do some additional benchmarking with compression of cache, and maybe some auto purge.

And you are really fast with merging. :) Thanks for your work!

[StephanGeorg]

FYI @jbeuckm @OlivierKamers

[swarnat]

We used this adjustment in our script now since 7 days without problems. We modified the cache to really only cache the tile

[StephanGeorg]

Hey @swarnat

Thank you for the PR, testing and sharing your experience with it. I haven't had time to test it yet, but I will do it asap and merge it into 1.7.0

✌️

[OlivierKamers]

Hi @swarnat!

That's a really nice idea and simple implementation 👍

A question I have is how you manage cleanup of expired items in the cache if they're not requested again? Do you run some sort of cron job outside staticmaps to periodically cleanup expired cache items?

[swarnat]

We implemented a nodejs webserver around this library, we will share soon. You are right, we implemented the cache cleanup on app level, but it is better placed within the library itself. There is a much better place, which can save runtime. Now it is included async in parallel of image generation. The cache cleanup happens in ~10% of all requests, because it is not needed more often.

We also did benchmarks about compression, but the little lower filesize wasn't useful in our case, because the longer response time was more important.

[StephanGeorg]

Maybe the automated cache cleanup should be optional. If you would have a huge cache it would've had bad impact to the map generation performance and running an external script to perform the cache cleanup could be more efficiant.

[swarnat]

After some time I come back to this PR. Sorry for delay.
I implemented an option "tileCacheAutoPurge" and added docs for function call map.clearCache()
We had this implementation in use since the implementation, 3 months ago, and got no problems.

[StephanGeorg]

After even more time (sorry for that), I reviewed your PR. First of all thanks for the implementation and testing. It is straightforward and effective improvement:

Benchmark

Test	no cache	with cache	improvement
render w/ center	246ms	76ms	69%
render w/ center from custom	239ms	94ms	60%
render w/ bbox	420ms	95ms	77%
render w/ icon	651ms	271ms	58%
render w/ remote url icon	629ms	276ms	56%
render w/o center	1793ms	864ms	52%
render w/o base layer	584ms	553ms	5%
render StaticMap w/ single polyline	807ms	244ms	69%
render StaticMap w/ polygon	406ms	161ms	60%
render StaticMap w/ multipolygon	305ms	102ms	66%
render StaticMap w/ thousands of polygons	10121ms	9529ms	6%
render StaticMap w/ circle	409ms	145ms	64%
render StaticMap with text	1446ms	823ms	43%
render text on NASA Blue Marble	6726ms	1271ms	81%
render w/ center	259ms	85ms	67%

Questions

• Why don't you use writeFileSync as you do with all other fs methods in getTiles? https://github.com/redoonetworks/staticmaps/blob/43409c06bb3b078a4b143554088f43c3e96e25ca/src/staticmaps.js#L663

• Could you also please add tests for the feature?

• [clearCache](#clearcache) | Adds text to the map clearCache method description in README is wrong

[swarnat]

Why don't you use writeFileSync as you do with all other fs methods in getTiles?

Because to write the cache file can be an asynchronous function and only would extend processing time of first call when synchronous Other option would be to integrate this into an own Promise, which then makes code more complex. But this brings me to a (maybe theoretically) problem, I tried to solve within commit: https://github.com/StephanGeorg/staticmaps/pull/60/commits/49975926f510cd02964cbe0718b27d64292ef4f5 Because when a cache file already exists, but isn't fully written, the cache data maybe is not consistent. So I added maybe too much checks, if data is correct. But more checks only bring more security, in my eyes.

Could you also please add tests for the feature?

Done: https://github.com/StephanGeorg/staticmaps/pull/60/commits/8a2339bbbeaa27370f0146a06a0c4a71575610c7 At the moment, these two tests are tests I would expect at minimum. I added chai-image package to compare image without cache and with cache. When you think, we need more tests, I can add. I never worked with mocha, so I'm not sure if it would be better structure to move "clearCache" function to a single file in utils folder. But because this function is only used within these tests and are direct dependencies, I integrated them into test group.

clearCache method description in README is wrong

Sorry. Updated

[StephanGeorg]

True, writeFileSync would block IO and initial request would take much longer to wait until disk write process is finished.

But maybe you should write the file to a temporary filename and rename it in the writeFile callback function to cacheKey. Then it's guaranteed that write process is finished. What you think?

[StephanGeorg]

Ah wait. But it wouldn't fix the fact that the 2nd request to that file has no info that it is currently in state "writing". So on top of that you also had to check if there's already the same temp. file name available and you had to wait until this file is renamed. Don't know if we're over engineering things here.

[swarnat]

We used this feature now since february for ~10 maps every day and do not recognize any errors. I just merged the latest version into fork, so merge conflicts are gone.

I also learned something new from your last commit: Async functions don't needs to return an extra Promise. That's great, because it decrease complexity of functions. You are free to merge this feature.

We also just upload our application to github, so everybody can quickly setup a nodejs / docker deployment with your library: https://github.com/redoonetworks/osm-static-maps-webserver