Context object cache #528

Merged
merged 8 commits into from Feb 25, 2013

3 participants

@kring
Analytical Graphics, Inc. member

It's a property bag that can be used to store arbitrary Context-tied objects and guarantee that they are destroyed (if necessary) when the Context is destroyed. This is a cleaner alternative in many cases to local property bags indexed on the Context's ID.

kring added some commits Feb 22, 2013
@kring kring Add a general cache to Context.
It's a property bag that can be used to store arbitrary Context-tied
objects and guarantee that they are destroyed (if necessary) when the
Context is destroyed.  This is a cleaner alternative in many cases to
local property
bags indexed on the Context's ID.
211c0f8
@kring kring Use Context cache to store 1 pixel water masks.
Also fix a typo in ViewportQuad.
eeccbdd
@kring kring Fix warning. eb0f89b
@pjcozzi
Analytical Graphics, Inc. member

Thanks, I'll look at this on Monday. This is the route I wanted to go. I think we can also use this in billboards. Did you see getDirectionsVertexBuffer and getIndexBuffer in BillboardCollection?

@kring
Analytical Graphics, Inc. member

No, I missed that in BillboardCollection. I'll update it there, too.

@mramato mramato commented on an outdated diff Feb 22, 2013
Source/Renderer/Context.js
@@ -254,6 +254,17 @@ define([
this._defaultTexture = undefined;
this._defaultCubeMap = undefined;
+
+ /**
+ * A cache of objects tied to this context. Just before the Context is destroyed,
+ * <code>destroy</code> will be invoked on each object in this object literal that has
+ * such a method. This is useful for caching any objects that might otherwise
+ * be stored globally, except they're tied to a particular context, and to manage
+ * their lifetime.
+ *
+ * @type {Object}
+ */
+ this.cache = {};
@mramato
Analytical Graphics, Inc. member
mramato added a line comment Feb 22, 2013

Property names for the cache don't seem to follow any kind of known naming scheme, so what about name collisions? One option is to list all of the "known names" and initialize them as undefined, this way we are also not modifying the literal at runtime (performance benefit). Another option would be to follow a well defined scheme, like objectName_propertyName.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato mramato and 1 other commented on an outdated diff Feb 22, 2013
Source/Scene/BillboardCollection.js
@@ -617,6 +607,26 @@ define([
return c.indexBuffer;
}
+ function getContextCache(context) {
+ // Per-context cache for billboard collections
+ var c = context.cache.billboardCollection;
+ if (typeof c === 'undefined') {
+ c = context.cache.billboardCollection = {
+ directionsVertexBuffer : undefined,
+ indexBuffer : undefined,
+ destroy : function() {
@mramato
Analytical Graphics, Inc. member
mramato added a line comment Feb 22, 2013

Technically, shouldn't all items in the cache also implement isDestroyed in order to properly follow the pattern?

@kring
Analytical Graphics, Inc. member
kring added a line comment Feb 22, 2013

I think it's unnecessary clutter. The contract with the Context is that, if it has a destroy method, the Context will call it. The Context doesn't need, nor will it call, an isDestroyed method. If your class that is adding something to the cache wants to query whether the object is destroyed, or if you're planning to share that object with the wider world, then adding an isDestroyed is a good idea. Otherwise, save yourself some typing and leave it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato mramato and 1 other commented on an outdated diff Feb 22, 2013
Source/Scene/BillboardCollection.js
@@ -617,6 +607,26 @@ define([
return c.indexBuffer;
}
+ function getContextCache(context) {
+ // Per-context cache for billboard collections
+ var c = context.cache.billboardCollection;
+ if (typeof c === 'undefined') {
+ c = context.cache.billboardCollection = {
@mramato
Analytical Graphics, Inc. member
mramato added a line comment Feb 22, 2013

Why not just store context.cache.billboardCollection_directionsVertexBuffer and context.cache.billboardCollection_indexBuffer in the cache directly. It would avoid an extra layer of unneeded indirection.

@kring
Analytical Graphics, Inc. member
kring added a line comment Feb 22, 2013

I guess I was thinking it was useful to minimize the number of properties in that cache.. but probably not, really. So I did what you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato
Analytical Graphics, Inc. member

Whatever unit tests we have for Context.destroy() should be updated (or created) to add some items to the cache and make sure they are properly destroyed on Context destroy.

@kring
Analytical Graphics, Inc. member

Property names for the cache don't seem to follow any kind of known naming scheme, so what about name collisions?

I named all the properties starting with the name of the class that added them, which I thought would make collisions reasonably unlikely.

@mramato
Analytical Graphics, Inc. member

Okay, I see that now. I wasn't immediately obvious since there wasn't anything separating the class name from the property name. Fine by me then.

@kring
Analytical Graphics, Inc. member

This is ready for another look.

@mramato
Analytical Graphics, Inc. member

This is fine with me. @pjcozzi, since you wanted to have a look at this, merge when you're happy.

@pjcozzi
Analytical Graphics, Inc. member

@kring this is good with me, but one question before I merge: do we want to document cache as @private for better encapsulation and protection against collisions? I can go either way since other folks will need something similar, but this isn't robust enough for the final solution. Perhaps we should err on the side of private before public.

@kring
Analytical Graphics, Inc. member

I'm ok with making it private for now, but what do you think a more robust solution would look like? I suppose we could use accessor functions instead of a property, and using more obfuscated names. If that's the sort of thing you'd like to see I can do it now.

@kring
Analytical Graphics, Inc. member

I should mention that while I'm not opposed to accessor-based thing I mentioned above, I feel like it is overkill. Cesium and Javascript require a certain amount of trust on the part of the developers using them. There's nothing stopping me from clobbering an important Context method, for example. But we trust people aren't going to do that, just like we can probably trust that people won't mess with other people's stuff in the cache. The naming convention based on the name of the type adding to the cache should make accidental collisions unlikely for practical purposes.

@pjcozzi
Analytical Graphics, Inc. member

Let's just make it @private for now.

Longer term, I could see classes registering themselves once to prevent collisions or the cache/context provided to the primitive's constructor function, e.g., think addEllipsoid replacing new and add.

@kring
Analytical Graphics, Inc. member

Alrighty, it's private.

@pjcozzi
Analytical Graphics, Inc. member

Merging.

@pjcozzi pjcozzi merged commit 1c9e463 into master Feb 25, 2013
@pjcozzi pjcozzi deleted the contextObjectCache branch Feb 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment