Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drawables should have build-based cache id #172

Closed
magicseth opened this issue Oct 4, 2014 · 10 comments
Closed

Drawables should have build-based cache id #172

magicseth opened this issue Oct 4, 2014 · 10 comments

Comments

@magicseth
Copy link

Drawables should not be cached by ints, as they can change at each compilation. Instead, they should be cached based on a combination of the specific build and the int identifier (or the resource name).

Just caching based on resource name alone is not sufficient, because the resource can change on each build also.

I'd recommend using the app signature to generate the cache key:
http://stackoverflow.com/questions/5578871/how-to-get-app-signature

FIXES: 17831665

@TWiStErRob
Copy link
Collaborator

I think the versionCode should be sufficient, based on the fact that you need to increment it every time you upload an apk to Play store (regardless of alpha/beta/prod).
That in itself should solve your issue assuming you're using applicationIdSuffix for debug.

@magicseth
Copy link
Author

That will lead to strange bugs in development, as each install will potentially show the wrong drawables.

@TWiStErRob
Copy link
Collaborator

Ah, very true. I wasn't thinking about continuous build/run cycles just from the perspective of what a user may experience.

A concern: that means that each debug deploy will have a differrent id, even if no drawable changed, so it'll always be a cache miss.

Based on the above two I think it's better to clear the cache when drawables changed.

What's that "FIXES:" in the report?

@magicseth
Copy link
Author

Another option would be to not even cache drawable at all, as presumably,
they are already on disk locally.

The fixes is just a reference to the bug in my internal bug tracker so I
can keep track when this is resolved 😎

@sjudd
Copy link
Collaborator

sjudd commented Oct 4, 2014

This ties into a larger issue we have where we generically cache content at Uris (#174). For drawables we can either, as @magicseth suggests, not cache content all or we could also probably get away just caching drawables in the memory cache. Unlike most Uris, the only way the drawable id would change would be on re-install so the memory cache should be safe.

I'd also be totally open to trying to mix something into the disk cache key for Drawables, but it would have to work for developers as well as end users and I can't think of anything that changes after each compilation?

@magicseth
Copy link
Author

I think the hash of the signature will change each install. But honestly,
the memory cache is probably fine.

@sjudd
Copy link
Collaborator

sjudd commented Oct 5, 2014

I tested Signatures, they're consistent across installs/compilation. Signatures are used to validate permissions so I'd assume they're tied to the key used to sign the application and shouldn't change.

For now I think I'll do just versionCode + resource name (rather than id), which will make things much more consistent. We can address broader concerns about caching data at uris in #174

@sjudd sjudd added the bug label Oct 5, 2014
@sjudd sjudd added this to the 3.4 milestone Oct 5, 2014
@TWiStErRob
Copy link
Collaborator

How about getting the hash of the .apk file?

They're reading the whole file into memory but it would be more efficient to call .update with the buffer in each iteration.

Would it be a big deal to provide a public interface with 1-2 default implementations mentioned above and expose something in the Glide builder for example to set it? That way the default could be versionCode, but extremists can use the apk hash or something else.

sjudd added a commit to sjudd/glide that referenced this issue Oct 5, 2014
@sjudd
Copy link
Collaborator

sjudd commented Oct 6, 2014

The hash of the apk (or maybe more precisely the drawable in question?) is certainly the most correct, but, at least currently, glide requires that cache keys are generated synchronously on the main thread, which rules out IO. We could change that but then memory cache keys wouldn't be invalidated at the same time as disk cache keys which would open a different can of worms.

I'm inclined to stick with name and version code for now which will make things mostly consistent and focus on trying to solve the problem generally for all Uris.

@sjudd sjudd mentioned this issue Oct 7, 2014
@sjudd
Copy link
Collaborator

sjudd commented Oct 10, 2014

Going to call this fixed in #179.

It's not perfect for developers, but the switch to resource names in sjudd@a1912e2 should help mitigate that, and for end users results will be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants