Closed
Bug 988186
Opened 11 years ago
Closed 11 years ago
HTTP cache v2: store all fields in CacheFileMetadata in network order on disk
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: michal, Assigned: michal)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8397070 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
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...
Assignee | ||
Comment 5•11 years ago
|
||
(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
Flags: needinfo?(michal.novotny) → needinfo?(honzab.moz)
Comment 6•11 years ago
|
||
(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
Flags: needinfo?(honzab.moz)
Comment 7•11 years ago
|
||
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.
Attachment #8397070 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 8•11 years ago
|
||
(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).
Attachment #8397070 -
Attachment is obsolete: true
Attachment #8398525 -
Flags: review?(honzab.moz)
Comment 9•11 years ago
|
||
(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.
Updated•11 years ago
|
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 10•11 years ago
|
||
(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.
Attachment #8398525 -
Attachment is obsolete: true
Attachment #8398525 -
Flags: review?(honzab.moz)
Attachment #8400242 -
Flags: review?(honzab.moz)
Flags: needinfo?(michal.novotny)
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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?
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 14•11 years ago
|
||
(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().
Flags: needinfo?(michal.novotny)
Comment 15•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #8400242 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8400242 -
Attachment is obsolete: true
Attachment #8402738 -
Flags: review+
Assignee | ||
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•