988186 - HTTP cache v2: store all fields in CacheFileMetadata in network order on disk

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8397070 [details] [diff] [review]
patch v1

cache2=off   https://tbpl.mozilla.org/?tree=Try&rev=4a234027b199
cache2=on    https://tbpl.mozilla.org/?tree=Try&rev=e306360d5ab4

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8397070 [details] [diff] [review]
patch v1

Review of attachment 8397070 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFileMetadata.h
@@ +25,5 @@
>    ((uint32_t)(aFrecency * CacheObserver::HalfLifeSeconds()))
>  #define INT2FRECENCY(aInt) \
>    ((double)(aInt) / (double)CacheObserver::HalfLifeSeconds())
>  
> +struct CacheFileMetadataHeader {

class

@@ +41,5 @@
> +    NetworkEndian::writeUint32(ptr, mLastFetched); ptr += sizeof(uint32_t);
> +    NetworkEndian::writeUint32(ptr, mLastModified); ptr += sizeof(uint32_t);
> +    NetworkEndian::writeUint32(ptr, mFrecency); ptr += sizeof(uint32_t);
> +    NetworkEndian::writeUint32(ptr, mExpirationTime); ptr += sizeof(uint32_t);
> +    NetworkEndian::writeUint32(ptr, mKeySize);

why not rather cast the buffer to CacheFileMetadataHeader and store to the respective members?  if there is no strong reason not to then I would prefer it

or are you worried about alignment or different size of the buffer?  if so then you probably cannot use p += sizeof(CacheFileMetadataHeader); on the call sites anyway.

is sizeof(CacheFileMetadataHeader) same on all platform and bit depths?

another reason we need some good buffer encapsulation.  you could just do something like NetworkEndian::writeUint32(buffer.write<uint32_t>(), mXXX) and be perfectly safe...

Comment 4

[Honza Bambas (:mayhemer)]

Comment 3

Comment 5

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> why not rather cast the buffer to CacheFileMetadataHeader and store to the
> respective members?  if there is no strong reason not to then I would prefer
> it

This is a recommended way taken from http://mxr.mozilla.org/mozilla-central/source/mfbt/Endian.h#40

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #5)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > why not rather cast the buffer to CacheFileMetadataHeader and store to the
> > respective members?  if there is no strong reason not to then I would prefer
> > it
> 
> This is a recommended way taken from
> http://mxr.mozilla.org/mozilla-central/source/mfbt/Endian.h#40

Can you please double check that sizeof(CacheFileMetadataHeader) == sum of sizeof() of all its elements?  Mainly on a 64bit build.

If not, then we have a problem - you are moving the buffer ptr too far at [1], so it won't be possible w/o a compatibility problems migrate to the API from bug 966024.  And probably doesn't make the cache entries platform independent even with the current format.  We should keep them such when the effort would be reasonable.  Here I think it is.

[1] http://hg.mozilla.org/mozilla-central/annotate/bb4dd9872236/netwerk/cache2/CacheFileMetadata.cpp#l235

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8397070 [details] [diff] [review]
patch v1

Review of attachment 8397070 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but please address (or discuss if you don't agree) the comment.

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +235,1 @@
>    p += sizeof(CacheFileMetadataHeader);

So, to move forward here.  I suggest the following:

since this is changing the format anyway, instead of shifting the size here by sizeof(CacheFileMetadataHeader) you should pass the p by reference to mMetaHdr.WriteToBuf() so that it will be shifted by an exact number of octets added to the buffer.  This way we will be completely independent on any cross-platform differences in alignment/class sizing.

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v2

(In reply to Honza Bambas (:mayhemer) from comment #7)
> since this is changing the format anyway, instead of shifting the size here
> by sizeof(CacheFileMetadataHeader) you should pass the p by reference to
> mMetaHdr.WriteToBuf() so that it will be shifted by an exact number of
> octets added to the buffer.  This way we will be completely independent on
> any cross-platform differences in alignment/class sizing.

This isn't completely independent solution because we would not know how many bytes to allocate for mWriteBuf. Have a look at the new patch, this should work on any platform and there is also a check of sizeof(CacheFileMetadataHeader).

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #8)
> Created attachment 8398525 [details] [diff] [review]
> patch v2
> 
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > since this is changing the format anyway, instead of shifting the size here
> > by sizeof(CacheFileMetadataHeader) you should pass the p by reference to
> > mMetaHdr.WriteToBuf() so that it will be shifted by an exact number of
> > octets added to the buffer.  This way we will be completely independent on
> > any cross-platform differences in alignment/class sizing.
> 
> This isn't completely independent solution because we would not know how
> many bytes to allocate for mWriteBuf. Have a look at the new patch, this
> should work on any platform and there is also a check of
> sizeof(CacheFileMetadataHeader).

Why should the buffer allocation be a problem here?  You know you need the sum of sizes of elementary-types of the structure.  But yes, this is what bug 966024 is all about...

I looked into the patch, better for this is to use MOZ_STATIC_ASSERT.  Sum the member sizes and equal it to the size of the structure.

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v3

(In reply to Honza Bambas (:mayhemer) from comment #9)
> Why should the buffer allocation be a problem here?  You know you need the
> sum of sizes of elementary-types of the structure.

Sure, but we would need to replace all sizeof(CacheFileMetadataheader) with call to a new method CacheFileMetadataHeader::Size() which we would need to keep up to date whenever we change the header. I don't find this to be an elegant solution.


> But yes, this is what bug 966024 is all about...

OK, let's change it in that bug and keep this patch as simple as possible.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #10)
> Created attachment 8400242 [details] [diff] [review]
> patch v3
> 
> (In reply to Honza Bambas (:mayhemer) from comment #9)
> > Why should the buffer allocation be a problem here?  You know you need the
> > sum of sizes of elementary-types of the structure.
> 
> Sure, but we would need to replace all sizeof(CacheFileMetadataheader) with
> call to a new method CacheFileMetadataHeader::Size() which we would need to
> keep up to date whenever we change the header. I don't find this to be an
> elegant solution.

I could well say there is something wrong with your design.  Structure sizes and persistence don't go well together.  You could always have a static const with size counted from the elements if you need it.  But that is not nice either.  Hard to say...  Let's move forward here with what we have.

> 
> 
> > But yes, this is what bug 966024 is all about...
> 
> OK, let's change it in that bug and keep this patch as simple as possible.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8400242 [details] [diff] [review]
patch v3

Review of attachment 8400242 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +228,5 @@
>                  sizeof(CacheFileMetadataHeader) + mKey.Length() + 1 +
>                  mElementsSize + sizeof(uint32_t)));
>  
>    char *p = mWriteBuf + sizeof(uint32_t);
>    memcpy(p, mHashArray, mHashCount * sizeof(CacheHash::Hash16_t));

Don't you have the same problem here?

@@ +775,3 @@
>      LOG(("CacheFileMetadata::ParseMetadata() - Metadata hash mismatch! Hash of "
>           "the metadata is %x, hash in file is %x [this=%p]", hash,
> +         NetworkEndian::readUint32(mBuf + aBufOffset), this));

can you please save this to a local var instead of doing twice?

::: netwerk/cache2/CacheFileMetadata.h
@@ +69,5 @@
> +  {
> +    static_assert((sizeof(mFetchCount) + sizeof(mLastFetched) +
> +      sizeof(mLastModified) + sizeof(mFrecency) + sizeof(mExpirationTime) +
> +      sizeof(mKeySize)) == sizeof(CacheFileMetadataHeader),
> +      "Unexpected sizeof(CacheFileMetadataHeader)!");

looks, good.  please use MOZ_STATIC_ASSERT and put it outside the class definition.  This is evaluated at compile time so having and calling this method is somewhat meaningless.

Examples: http://mxr.mozilla.org/mozilla-central/ident?i=MOZ_STATIC_ASSERT&filter=

Comment 13

[Honza Bambas (:mayhemer)]

Emphasizing the one important question this review is stuck on:

::: netwerk/cache2/CacheFileMetadata.cpp
@@ +228,5 @@
>                  sizeof(CacheFileMetadataHeader) + mKey.Length() + 1 +
>                  mElementsSize + sizeof(uint32_t)));
>  
>    char *p = mWriteBuf + sizeof(uint32_t);
>    memcpy(p, mHashArray, mHashCount * sizeof(CacheHash::Hash16_t));

Don't you have the same problem here?

Comment 14

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> >    char *p = mWriteBuf + sizeof(uint32_t);
> >    memcpy(p, mHashArray, mHashCount * sizeof(CacheHash::Hash16_t));
> 
> Don't you have the same problem here?

What problem do you exactly mean? Size of the hash array or the network order? Values are converted to/from network order directly in Set/GetHash().

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > >    char *p = mWriteBuf + sizeof(uint32_t);
> > >    memcpy(p, mHashArray, mHashCount * sizeof(CacheHash::Hash16_t));
> > 
> > Don't you have the same problem here?
> 
> What problem do you exactly mean? Size of the hash array or the network
> order? Values are converted to/from network order directly in Set/GetHash().

Perfect!  r+ then.

Comment 16

[Michal Novotny [:michal]]

Attachment: patch v4

Comment 17

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4481bf1cf4c1

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4481bf1cf4c1