Put the cache entry format version number in each cache entry so we can change the format of cache entries without blowing away the entire cache each time we do so

RESOLVED DUPLICATE of bug 941698

Status

()

RESOLVED DUPLICATE of bug 941698
7 years ago
4 years ago

People

(Reporter: briansmith, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

+++ This bug was initially created as a clone of Bug #715198 +++

We should create a new version number to keep track of the format of cache entries. Whenever we change the format of entries in the cache, we must increase this version number. When we write a cache entry, we should write a version number in the entry. It must be possible to read this version number from the entry without parsing the rest of the entry.

When we read a cache entry, if it has a cache entry version number that we don't understand, then we should avoid parsing it, doom it, and return "not found." But, we should try to maintain backward compatibility with older versions of the cache entry format, whenever possible. However, we shouldn't require this backward-compatibility for every change. For example, for a change like bug 697781, it isn't realistic to maintain backward compatibility. Whenever we break backward compatibility in these cases, we should remove all the code that implements the backward compatibility.

The current "cache version number" would then represent the version number of the format of everything except the entries themselves. We should NOT increase the existing "cache version number" in cases where only the format of entries has changed.

This way, we can avoid deleting the entire cache every time we change the format of entries. We would still have to blow away the cache when we change the format of other on-disk data structures.

One exception: We will have to increase the cache version number when we fix *this* bug, because older versions of Firefox wouldn't know how to check the cache entry version number field before parsing the rest of the entry.
I think we should fix this bug 715752 ASAP, especially before we make more changes to the format of cache entries, so I made this block the other bugs I know about that have patches change the cache entry format.
Blocks: 697781, 712914

Comment 2

7 years ago
Fwiw I'm not in favor of this. It may help to clean up old caches in a "softer" manner than we do at the moment but that is approached from other angles, afaik.

1) It adds one more layer of complexity to the logic in the cache; from the perspective of an efficient (and correct) cache this is not what we need

2) We rarely need to bump versions anyway; it seems like overkill to implement full support for something we rarely do

3) Cleaning the cache once-in-a-while is probably good in any case...

4) Separate profiles seems like a better way to experiment with different builds

If we chose to do this anyway I propose to keep the current versioning-logic for those changes which really need to blow the cache (like this bug) and rather implement a "property" (or "capability") based scheme for individual entries. I.e. each entry store some number of bits indicating "properties" of the entry and a browser-build uses only entries whose "properties" it has the capability to handle. (It's not too different from Brians original suggestion but it is more flexible than version-numbers.)
My scheme in bug 715198 comment 12 is another possible solution here?  It's simpler, but less flexible than Brian's scheme.
(In reply to Bjarne (:bjarne) from comment #2)
> 1) It adds one more layer of complexity to the logic in the cache; from the
> perspective of an efficient (and correct) cache this is not what we need

This is a fair point but...

> 2) We rarely need to bump versions anyway; it seems like overkill to
> implement full support for something we rarely do

I believe that we are likely to change the disk format of the cache fairly frequently--probably in every release or almost every release in the next few quarters.

> 3) Cleaning the cache once-in-a-while is probably good in any case...

Why? If this is true, then we should fix it so it isn't true. It is critical that Firefox doesn't slow down when you upgrade it. Resetting the cache on upgrade will slow Firefox down, so we shouldn't do it.

> 4) Separate profiles seems like a better way to experiment with different
> builds

I pretty much agree with this. But, the user using multiple versions of Firefox on the same cache is not the case I am worried about. I am more worried about the case where the user is upgrading from Firefox N to Firefox (N+1) and has cache full of useful stuff that we need to be able to use even for the very first request after starting Firefox (N+1). 

> If we chose to do this anyway I propose to keep the current versioning-logic
> for those changes which really need to blow the cache (like this bug) and
> rather implement a "property" (or "capability") based scheme for individual
> entries. I.e. each entry store some number of bits indicating "properties"
> of the entry and a browser-build uses only entries whose "properties" it has
> the capability to handle. (It's not too different from Brians original
> suggestion but it is more flexible than version-numbers.)

I think that sounds like a great improvement to my idea.
(In reply to Brian Smith (:bsmith) from comment #4)
> > implement a "property" (or "capability") based scheme for individual
> > entries. I.e. each entry store some number of bits indicating "properties"
> > of the entry and a browser-build uses only entries whose "properties" it has
> > the capability to handle. (It's not too different from Brians original
> > suggestion but it is more flexible than version-numbers.)
> 
> I think that sounds like a great improvement to my idea.

What if the properties' format changes?

Comment 6

7 years ago
(In reply to Honza Bambas (:mayhemer) from comment #5)
> What if the properties' format changes?

Then we bump the cache-version. But this can be implemented by a bitmask, i.e. let each bit mean that the entry requires a certain capability. A build has its own map and if an entry requires some capability (has a bit enabled which the build has not) then the build dooms the entry and loads from network. This gives compatibility in both directions plus it allows to play with any special processing for cache-entries while using existing caches. Example: We can use bit #0 to indicate that the entry is compressed (and hence the build must support compressed entries), bit #1 to indicate efficient TLS-storage, etc.

There are two snags though: 1) we must bump the cache-version to get started with this (as Brian points out in the end of comment #0), and 2) we are limited to a fixed number of capabilities depending on which datatype we chose to hold the bits (this should be part of the in-memory description for a cache-entry).

Comment 7

7 years ago
Brian, I really don't buy the cache-of-useful stuff argument. Cache is meant to be a lossy storage option. If you actually look at our cache speeds, they are very often not much better than fetching the page from internet. We should not be constraining based on that. 

I am not convinced of the utility of per-entry versioning. It seems to solve the compressed problem, but I doubt we'll have many more format changes like this. I think we should fix our cache invalidation/expiration to not do so much damn disk io. Ie there is no pressing need to delete the old cache entries. We can just rename() + ftruncate expired entries when allocating new ones(among other options).
the simplest thing I can think of is to invalidate the cache when a profile goes backwards between version numbers. That way you only need to store the version once, not per cache entry. That would leave the cache in tact for the vast majority of users who only go forward and also make it operate correctly for everyone.

I agree with Taras that invalidation is the least of the cache's problems - but Brian has a point that politically we should be reducing any negative impressions that go with upgrades given our recent increase in the rate of upgrades :) So its worth doing something quick and dirty - even if it doesn't help this instance.
(In reply to Taras Glek (:taras) from comment #7)
> If you actually look at our cache speeds, they are very often not
> much better than fetching the page from internet.

We shouldn't design things assuming that problem is permanent. 

> I am not convinced of the utility of per-entry versioning. It seems to solve
> the compressed problem, but I doubt we'll have many more format changes like
> this.

We will have the same problem when we fix the bugs depending on this one. Also, I think it is likely that we will change even the compression format again. (The compressed form of entries could do a lot better job than the current implementation does. This is just the first iteration of cache entry compression.) And, I would be surprised if we didn't come up with new changes to the entry format when we meet about it in a few weeks. So, we're looking at changing the format of entries in the cache at least two releases in a row, and probably more.

(In reply to Patrick McManus [:mcmanus] from comment #8)
> the simplest thing I can think of is to invalidate the cache when a profile
> goes backwards between version numbers. That way you only need to store the
> version once, not per cache entry. That would leave the cache in tact for
> the vast majority of users who only go forward and also make it operate
> correctly for everyone.

I think what you are saying is that we should bump the cache version number for each change, but instead of blowing away the cache, we either try to support entries written in the older format or remove them when we encounter them. I think that also seems reasonable. The current code (with compressed entry support) can cope with uncompressed entries for the mime types that are compressed already. But I bet it will require versioning information in the entries for other changes; for example, the change to the format of SSL info will require us to be able to distinguish between old and new formats of entries. But, that is a problem that could be solved later.

> I agree with Taras that invalidation is the least of the cache's problems -
> but Brian has a point that politically we should be reducing any negative
> impressions that go with upgrades given our recent increase in the rate of
> upgrades :) So its worth doing something quick and dirty - even if it
> doesn't help this instance.

My point is that it doesn't make sense to say "Firefox 55 is XX% faster than Firefox 54" but then have actual page load speed decrease immediately after upgrade because we effectively have no disk cache. Even though blowing away the disk cache is a temporary setback for the user, it is one that occurs at a time when they are especially likely to be judging our performance.
No longer blocks: 712914
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 941698
You need to log in before you can comment on or make changes to this bug.