Add caching to sampleTerrain #6284

hpinkos · 2018-02-27T22:56:39Z

Added LRUCache, a data structure for managing a ordered list based on what was most recently accessed. It has a DoublyLinkedList for keeping track of the order, and a dictionary for fast lookups. It has a capacity option so the size doesn't grow beyond the capacity, and an expiration option to clean out old entries when LRUCache.prune is called
Added DoublyLinkedList.addFront for pushing nodes to the front of the list
Added DoublyLinkedList.moveToFront for moving an existing node to the front of the list
Added DoublyLinkedList.removeAfter for removing all nodes including and after a given index
Updated sampleTerrain so it stores recently fetched tiles in an LRUCache. It sets the cache size limit to 256 entries, and sets the cache entries to expire and get cleared out after 10 seconds without being accessed.

cesium-concierge · 2018-02-27T22:56:39Z

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-02-27T23:03:30Z

Here's some sample code I used for testing this:

var worldTerrain = Cesium.createWorldTerrain({
    requestWaterMask: true,
    requestVertexNormals: true
});

var viewer = new Cesium.Viewer('cesiumContainer', {
    terrainProvider: worldTerrain
});

Sandcastle.addToolbarButton('Sample', function() {
    Cesium.sampleTerrainMostDetailed(worldTerrain, [Cesium.Cartographic.fromDegrees(40, 40)]);
});

Sandcastle.addToolbarButton('Sample random', function() {
    Cesium.sampleTerrainMostDetailed(worldTerrain, [Cesium.Cartographic.fromDegrees(Math.random() * 80, Math.random() * 80)]);
});

hpinkos · 2018-03-09T18:32:30Z

This is ready for review

… skip]

hpinkos · 2018-03-15T16:47:21Z

Could someone review this please? @bagnell or @lilleyse maybe?
This change is related to #6214. #6214 calls sampleTerrainMostDetailed to get the height relative to terrain, and this change keeps sampleTerrain from issuing an http request each time it's called.

lilleyse · 2018-03-15T17:47:44Z

I'll start taking a look.

lilleyse · 2018-03-15T17:20:32Z

Source/Core/DoublyLinkedList.js

+
+        var i;
+        var node = this.head;
+        for (i = 0; i < startIndex; i++) {


Style suggestions:
Remove var i; and declare it in the loop.
Change i++ to ++i

lilleyse · 2018-03-15T17:29:14Z

Source/Core/LRUCache.js

+    requestAnimationFrame,
+    DeveloperError,
+    DoublyLinkedList) {
+    'use strict';


Fix indentation for includes.

lilleyse · 2018-03-15T17:30:28Z

Source/Core/LRUCache.js

+    './defineProperties',
+    './getTimestamp',
+    './requestAnimationFrame',
+    './DeveloperError',


Minor point, but requestAnimationFrame isn't in alphabetical order.

lilleyse · 2018-03-15T17:50:35Z

Source/Core/sampleTerrain.js

+                requestPromise = tileRequest.terrainProvider.requestTileGeometry(tileRequest.x, tileRequest.y, tileRequest.level);
+                cache.set(cacheKey, requestPromise);
+            }
+            var tilePromise = when(requestPromise)


Is when( needed?

lilleyse · 2018-03-15T17:53:51Z

Specs/Core/LRUCacheSpec.js

+], function(
+    LRUCache,
+    getTimestamp) {
+    'use strict';


Fix indentation for includes.

lilleyse · 2018-03-15T17:55:38Z

Source/Core/LRUCache.js

+     * @alias AssociativeArray
+     * @constructor
+     */
+    function LRUCache(capacity, expiration) {


I'm actually not sure.. in Cesium is the preferred style to put the constructor first always or okay to have helpers above it?

I've always done it this way.
But I guess it does say constructor first in the style guide: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#put-the-constructor-function-at-the-top-of-the-file

Ok, good to know.

It looks like DoublyLinkedList was fixed but not LRUCache.

Whoops! All fixed now

lilleyse · 2018-03-15T17:56:37Z

Source/Core/LRUCache.js

+     * A cache for storing key-value pairs
+     * @param {Number} [capacity] The capacity of the cache.  If undefined, the size will be unlimited.
+     * @param {Number} [expiration] The number of milliseconds before an item in the cache expires and will be discarded when LRUCache.prune is called.  If undefined, items do not expire.
+     * @alias AssociativeArray


AssociativeArray -> LRUCache

lilleyse · 2018-03-15T17:57:02Z

Source/Core/LRUCache.js

+     * A cache for storing key-value pairs
+     * @param {Number} [capacity] The capacity of the cache.  If undefined, the size will be unlimited.
+     * @param {Number} [expiration] The number of milliseconds before an item in the cache expires and will be discarded when LRUCache.prune is called.  If undefined, items do not expire.
+     * @alias AssociativeArray


Should this class be private?

It probably doesn't have to be, but it isn't really useful in the Cesium API so I decided to make it private

lilleyse · 2018-03-15T18:40:51Z

Source/Core/LRUCache.js

+            prune(cache);
+
+            if (cache.length > 0) {
+                requestAnimationFrame(loop);


I'm not sure that it's good practice to call requestAnimationFrame here or anywhere besides the main render loop in CesiumWidget. I don't think it will break anything, but it may request a render that isn't desired.

At the same time, I'm not sure what the alternative would be. Is there any way for the lru cache to be owned by something with access to frame state?

I originally had Scene.update tell sampleTerrain to call cache.checkExpiration, but that results in that function being called every frame. This way, LRUCache can manage it's own expiration and doesn't execute the loop when there's nothing in the cache.

I agree that LRUCache is cleaner when it manages itself, I'm just worried about the calls to requestAnimationFrame.

Would it be not completely terrible to call prune after each set call? Or maybe use setTimeout if it doesn't need to prune every frame?

I prefer what @hpinkos has done here. It will not trigger a Cesium render, it will simply let LRUcache does it's processing all at once on the next browser (not Cesium) render

My concern is that it will trigger a browser render unnecessarily. Or maybe it won't... maybe that's going to happen every frame regardless. But I still think requestAnimationFrame really only belongs inside the main render loop since that's where we "ask" for a frame. But if this is the cleanest way to get the job done, then I suppose it's ok.

Two week later update. I read up on requestAnimationFrame more and it seems that it is purely a callback aggregator and it won't force a render like I thought might be happening. Sorry if this is already common knowledge...

I still think its nice that requestAnimationFrame is called in just one place, but technically it should be fine to call in multiple.

https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames

For the render loop, I agree incompletely, there should only be one requestAnimationFrame call. For anything we want to have happen independent of the render loop, multiple requestAnimationFrame make total sense. I think we need to continue to decouple things from our actual webgl render loop and move them one level up into requestAnimationFrame .

lilleyse · 2018-03-15T18:43:49Z

Specs/Core/LRUCacheSpec.js

+    });
+
+    it('set throws with undefined key', function() {
+        var associativeArray = new LRUCache();


Rename to cache.

hpinkos · 2018-03-15T20:09:05Z

@lilleyse ready for another look

hpinkos · 2018-03-15T22:01:12Z

@lilleyse anything else?

lilleyse · 2018-03-16T13:16:05Z

Nope that's it, thanks @hpinkos.

pjcozzi · 2018-03-31T01:03:57Z

I know the public API didn't change, but this improves performance for some cases, right? Update CHANGES.md?

pjcozzi · 2018-03-31T01:08:09Z

Source/Core/sampleTerrain.js

    'use strict';

+    var cache = new LRUCache(256, 10000);


Maybe this was discussed and this is the best solution - if so please point me to it, but this is not how I originally suggested to implement this.

We generally want to centralize all the time-related cache logic so I suggested that we use the render loop to flush any caches just like we do for the shader cache. Are we sure this setInterval approach is better? I think explicit and consistent control of caches in our render loop (but still based on time stamp) would be more cohesive.

There was some discussion here: #6284 (comment)

Since sampleTerrain doesn't have access to frameState some of the options were:

Scene edits a global property in sampleTerrain that tells it to flush the cache.

sampleTerrain uses requestAnimationFrame to hook into the render loop to flush the cache, making sampleTerrain self contained.

sampleTerrain uses setInterval instead of requestAnimationFrame.

One benefit of the third approach (which I guess is opposite your original suggestion) is that sampleTerrain is not tied to the render loop at all, making it possible to use in Node.js or wherever else the Cesium engine isn't actually running.

Ah I did not think about the standalone use case. I'm still not sure that a standalone API would implicitly cache and flush like this (imagine if an OpenGL driver did thinks like this); the user would likely have more fine-grained control and would be able to pass in a cache policy or whatever.

Still not convinced that having random caches with random set intervals is a good design in Cesium, but proceed as you see fit.

Still not convinced that having random caches with random set intervals is a good design in Cesium

I think everyone agrees with this sentiment. In my opinion what Cesium really needs is a shared centralized cache (just like we also need a centralized worker pool).

hpinkos · 2018-04-02T20:12:00Z

Do we want to revert this change? I don't think I need it for the things I'm working on after all. @pjcozzi @lilleyse @mramato

lilleyse · 2018-04-02T20:31:34Z

Personally I'm fine with keeping it as it may still be useful for other cases, and the caching system can be changed whenever we are ready. But if anyone wants to revert that sounds fine too.

Add caching to sampleTerrain

4820390

hpinkos added 3 commits March 8, 2018 11:31

Merge branch 'master' into cache-sampleterrain

40412b2

LRUCache expiration

26892f1

cleanup

f615296

hpinkos added 4 commits March 13, 2018 14:27

use requestRenderFrame instead of tying in to scene render loop [ci…

a2b7a28

… skip]

fix loop, change cache expiration [ci skip]

f8f2fa9

fix specs

5d8d240

fix doc

0b74558

lilleyse reviewed Mar 15, 2018

View reviewed changes

hpinkos added 2 commits March 15, 2018 15:55

Merge branch 'master' into cache-sampleterrain

6b38b9a

cleanup after review

e5f2e7b

Hannah added 3 commits March 15, 2018 16:29

move private functions

fd1bc99

fix indent

b0c7575

fix eslint

4ec3576

lilleyse merged commit 11bd15f into master Mar 16, 2018

lilleyse deleted the cache-sampleterrain branch March 16, 2018 13:16

pjcozzi reviewed Mar 31, 2018

View reviewed changes

hpinkos mentioned this pull request Apr 2, 2018

Remove LRUCache from sampleTerrain #6409

Merged

Add caching to sampleTerrain #6284

Add caching to sampleTerrain #6284

Conversation

hpinkos commented Feb 27, 2018 • edited Loading

cesium-concierge commented Feb 27, 2018

hpinkos commented Feb 27, 2018 • edited Loading

hpinkos commented Mar 9, 2018

hpinkos commented Mar 15, 2018

lilleyse commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Mar 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Mar 15, 2018

hpinkos commented Mar 15, 2018

lilleyse commented Mar 16, 2018

pjcozzi commented Mar 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Apr 2, 2018

lilleyse commented Apr 2, 2018

hpinkos commented Feb 27, 2018 •

edited

Loading

hpinkos commented Feb 27, 2018 •

edited

Loading

lilleyse Mar 31, 2018 •

edited

Loading