Decom nsICacheEntryInfo, replace by ExpirationTime attribute to nsICachingChannel

ASSIGNED
Assigned to

Status

()

defect
P3
normal
ASSIGNED
13 years ago
2 years ago

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Tracking

(Blocks 1 bug, {arch, memory-footprint})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
Note nsICacheEntryInfo is, apart from nsAboutCache, only used in about 6 locations to get the ExpirationTime and in 1 location to set the ExpirationTime.

May be the expirationTime should become an attribute of nsICachingChannel, so that it can be get/set directly from that, just like Get/SetCacheFromFile?
The only real implementor of nsICachingChannel is nsHttpChannel.

If ExpirationTime is an attribute to nsICachingChannel, the various callers (except AboutCache), can directly get/set that attribute from the CachingChannel.

/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp, line 306 -- nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken);
/browser/components/places/src/nsBookmarksFeedHandler.cpp, line 275 -- nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken);
/docshell/base/nsDocShell.cpp, line 7501 -- nsCOMPtr<nsICacheEntryInfo> cacheEntryInfo(do_QueryInterface(cacheToken));
/modules/libpr0n/src/imgRequest.cpp, line 644 -- nsCOMPtr<nsICacheEntryInfo> entryDesc(do_QueryInterface(cacheToken));
/uriloader/prefetch/nsPrefetchService.cpp, line 166 -- nsCOMPtr<nsICacheEntryInfo> entryInfo(do_QueryInterface(cacheToken, &rv));
/widget/src/mac/nsSound.cpp, line 410 -- nsCOMPtr<nsICacheEntryInfo> cacheEntryInfo = do_QueryInterface(cacheToken);

So, before:
    nsCOMPtr<nsICachingChannel> channel = do_QueryInterface(aRequest);
    if (channel) {
        nsCOMPtr<nsISupports> cacheToken;
        channel->GetCacheToken(getter_AddRefs(cacheToken));
        if (cacheToken) {
            nsCOMPtr<nsICacheEntryInfo> entryInfo = do_QueryInterface(cacheToken);
            if (entryInfo) {
                PRUint32 expiresTime;
                
                if (NS_SUCCEEDED(entryInfo->GetExpirationTime(&expiresTime))) {
                    ....
                }
            }
        }
    
After:
    nsCOMPtr<nsICachingChannel> channel = do_QueryInterface(aRequest);
    if (channel && (NS_SUCCEEDED(channel->GetExpirationTime(&expiresTime)))) {
        ....
    }

Bug 230675 is about cleaning up the cache visitor / cacheEntryInfo / cacheEntryDescriptor stuff, saving about 47KiB code.
This bug should be done first to add Get/SetExpirationTime to nsICachingChannel, making nsICacheEntryInfo redundant (except for in nsAboutCache).
(Assignee)

Comment 1

13 years ago
This patch adds GetExpirationTime to nsICachingChannel so that most of the callers of getCacheToken/CacheEntryInfo can now directly do GetExpirationTime.
Also made 'cacheToken' an readonly attribute, as the Setter was not implemented anyway.
(Assignee)

Comment 2

13 years ago
Posted patch V2: Also do browser part (obsolete) — Splinter Review
This patch makes GetExpirationTime a member of nsICachingChannel.
Which prevents the need to QI the cachetoken to nsICacheEntryInfo,
which in turn prevents the need for nsICacheEntryInfo completely.
This saves about 5K (!) per caller in code object size.
Also mCacheStream in the bookmark/places code is unused and thus removed.
Also the nsICacheVisitor.h include is not needed and is removed.
Attachment #215630 - Attachment is obsolete: true
Attachment #216850 - Flags: review?(darin)

Comment 3

13 years ago
Comment on attachment 216850 [details] [diff] [review]
V2: Also do browser part

>Index: netwerk/base/public/nsICachingChannel.idl

> [scriptable, uuid(b1f95f5e-ee05-4434-9d34-89a935d7feef)]
> interface nsICachingChannel : nsISupports
> {
>     /**
>+     * Get the expiration time of the cache entry (in seconds since the Epoch).
>+     */
>+    readonly attribute PRUint32 expirationTime;

When changing an interface, it is essential that you change the UUID
property of the interface.  Otherwise, you break binary compatibility
with previous versions of Mozilla, which can cause all kinds of havoc
for extension authors.


Index: browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp

>+            if (NS_SUCCEEDED(channel->GetExpirationTime(&expiresTime))) {
>+                PRInt64 temp64, nowtime = PR_Now();
>+                PRUint32 nowsec;
>+                LL_I2L(temp64, PR_USEC_PER_SEC);
>+                LL_DIV(temp64, nowtime, temp64);
>+                LL_L2UI(nowsec, temp64);
>+    
>+                if (nowsec >= expiresTime) {
>+                    expiresTime -= nowsec;
>+                    if (ttl < (PRInt32) expiresTime)
>+                        ttl = (PRInt32) expiresTime;
>                 }

There is no need to use the LL_ macros anymore.  You can assume that
the compiler supports PRInt64/PRUint64 arithmetic.


r=darin w/ the UUID change.  please post a new patch for check-in.
Attachment #216850 - Flags: review?(darin) → review-
(Assignee)

Comment 4

13 years ago
Note: the TTL calculation is strange:
+        if (nowsec >= expiresTime) {
+          expiresTime -= nowsec;
+          if (ttl < (PRInt32) expiresTime)
+            ttl = (PRInt32) expiresTime;
That means that if nowsec (currenttime) is past the expiresTime, the expiresTime becomes negative (but stored as unsigned so very big!), and the TTL then becomes very big?
Attachment #216850 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #219586 - Flags: review?(darin)
(Assignee)

Comment 5

13 years ago
Darin, can you take a look at this?
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #219586 - Flags: review?(darin.moz) → review?(dcamp)
(Assignee)

Comment 6

12 years ago
This patch needs to be unbitrotted, but is still valid in concept, simplifying the cache interface.
Assignee: nobody → alfredkayser
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
No longer blocks: 230675
Blocks: deCOM
Is this bug still relevant?
(Assignee)

Comment 8

9 years ago
It is still very relevant, as this is DECOM, and general code simplification.
(Assignee)

Comment 9

9 years ago
Comment on attachment 219586 [details] [diff] [review]
V3: New UUID and replaced LL calculations

Probably bitrotted, but still useful in making cache code simpler.
Attachment #219586 - Flags: review?(dcamp) → review?(bzbarsky)
I'm really not a good reviewer for the cache stuff; I'm not that familiar with it.  Perhaps Michal?  Or maybe Jason can recommend someone?
(Assignee)

Updated

9 years ago
Depends on: 537164
Attachment #219586 - Flags: review?(bzbarsky) → review?(jduell.mcbugs)
James, ping for review please - or if you're not the right person to review/don't have the time to review at the moment, please can you tell me who else to ask - thanks! :-)

Comment 12

8 years ago
Comment on attachment 219586 [details] [diff] [review]
V3: New UUID and replaced LL calculations

I'll let bjarne or michal (or another cache person) take this
Attachment #219586 - Flags: review?(jduell.mcbugs)
Attachment #219586 - Flags: review?(bjarne)

Comment 13

7 years ago
Comment on attachment 219586 [details] [diff] [review]
V3: New UUID and replaced LL calculations

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

Patch has bitrotted and e.g. uses PRBool everywhere - please fix. Also, nsBookmarksFeedHandler.cpp does not seem to exist anymore - logic seems to have moved to nsLivemarkService.js. Moreover, I observe that even more cpp-files should be updated with this improvement - you may want to do another search like in comment #0 to find them.

I'm giving this an r- because there are a number of small issues plus the patch has bitrotted, but the approach and code is generally fine. Please re-request review when patch has been updated.

::: docshell/base/nsDocShell.cpp
@@ +7460,5 @@
>           */
>          if (cacheChannel) {
>              cacheChannel->GetCacheKey(getter_AddRefs(cacheKey));
> +            PRUint32 expTime;
> +            cacheChannel->GetExpirationTime(&expTime);

Please handle error-returns, or comment / indicate that you don't care about it. Note that |expTime| has no default value.

@@ +7496,5 @@
>       */    
>      if (discardLayoutState) {
>          entry->SetSaveLayoutStateFlag(PR_FALSE);
>      }
> +    if (expired == PR_TRUE) {

Drop `==`

::: modules/libpr0n/src/imgRequest.cpp
@@ +639,5 @@
>      nsCOMPtr<nsICachingChannel> cacheChannel(do_QueryInterface(aRequest));
>      if (cacheChannel) {
> +      PRUint32 expiration;
> +      /* get the expiration time from the caching channel's token */
> +      cacheChannel->GetExpirationTime(&expiration);

Please handle error-returns here. Note also that all this logic has been moved to the SetCacheValidation() method.

::: widget/src/mac/nsSound.cpp
@@ +404,5 @@
>    // try to get the expiration time from the URI load
>    nsCOMPtr<nsICachingChannel> cachingChannel = do_QueryInterface(inChannel);
>    if (cachingChannel)
>    {
> +    cachingChannel->GetExpirationTime(&expirationTime);

Please handle error-returns, or comment / indicate that you don't care about it.

::: netwerk/base/public/nsICachingChannel.idl
@@ +71,5 @@
>       *
>       * The cache token can be QI'd to a nsICacheEntryInfo if more detail
>       * about the cache entry is needed (e.g., expiration time).
>       */
> +    readonly attribute nsISupports cacheToken;

I don't think this should be done as part of this patch (just register a new bug for it), but if you really want to make it readonly you need to clean up comment.

::: browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp
@@ +303,5 @@
> +                PRUint32 nowsec = (PRUint32)(PR_Now() / (PRInt64)PR_USEC_PER_SEC);
> +                if (nowsec >= expiresTime) {
> +                    expiresTime -= nowsec;
> +                    if (ttl < (PRInt32) expiresTime)
> +                        ttl = (PRInt32) expiresTime;

I agree with your comment #4 that this looks funny. However, thus stuff seems to have moved to nsLivemarkService.js and logic is different there.
Attachment #219586 - Flags: review?(bjarne) → review-
Whiteboard: [necko-backlog]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.