Closed Bug 485230 Opened 11 years ago Closed 11 years ago

Add way to disable caching during development

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

Details

Attachments

(1 file, 1 obsolete file)

Right now caching is on by default during development builds (if debug is on).  This can actually get in the way of writing tests, as the network traffic doesn't actually happen, and changes to header files, etc., may not happen.  Confusing.

I've added a patch that turns off caching in a development tree if NECKO_DEV_DISABLE_CACHE is set in the environment.   This seems better than turning it off during development by default, as we've got some cache tests that will presumably not get exercised (?) if caching is off.

I've taken away the #if DEBUG wrapper, so one effect of this patch is that caching will not be on by default for non-debug development trees.  Is that OK?
Attachment #369337 - Flags: superreview?(cbiesinger)
Attachment #369337 - Flags: review?(cbiesinger)
We also have a memory cache. That means:
- The tests that exercise caching do work, they use the memory cache (with a few special exceptions that set a profile dir or something, I think). Tinderboxes doesn't run tests in Debug mode AFAIK, so the tests need to support that :)
- This env variable is not named ideally, as it only affects the disk cache
- I don't think we should enable the disk cache in this case in opt builds. It has the effect that everyone running an opt xpcshell build will get Cache directories scattered around.
There's actually an even better reason not to enable this by default: Embeddors who don't set a profile should not get a disk cache directory forced upon them.
It sounds like you're saying we should never turn on disk caching by default in this block of code (i.e. if we don't find the parent cache directory using "regular" means), under either debug or opt builds, as debug builds will test caching via the memory cache, and opt builds don't want the disk cache directories lying around.  So I would change this patch to only turn on caching if some env var was set.  Right?

Sounds good in theory, but I'm a bit confused by the memory cache.  I'm loading a small, simple page, with no cache policy headers (so it should be cached, and it is if disk caching is on).  When I turn off disk caching, I then always see network traffic.  If memory caching is on, shouldn't we hit the memory cache instead of going over the network?  This is the one and only page load I do, from a freshly re-started browser.

> I've taken away the #if DEBUG wrapper, so one effect of 
> this patch is that caching will not be on by default for 
> non-debug development trees.  Is that OK?

Typo:  I should have said that caching *would* be on for non-debug development trees.  But you understood that that's what the code was doing.
(In reply to comment #3)
> It sounds like you're saying we should never turn on disk caching by default in
> this block of code (i.e. if we don't find the parent cache directory using
> "regular" means), under either debug or opt builds, as debug builds will test
> caching via the memory cache, and opt builds don't want the disk cache
> directories lying around.  So I would change this patch to only turn on caching
> if some env var was set.  Right?

That's exactly right.

> Sounds good in theory, but I'm a bit confused by the memory cache.  I'm loading
> a small, simple page, with no cache policy headers (so it should be cached, and
> it is if disk caching is on).  When I turn off disk caching, I then always see
> network traffic.  If memory caching is on, shouldn't we hit the memory cache
> instead of going over the network?  This is the one and only page load I do,
> from a freshly re-started browser.

Yes. I'm pretty sure that's how the code is supposed to work. Might be interesting to see an nsHttp:5 log of that.
Attached patch New fixSplinter Review
OK, I've change the patch so that we now only turn on caching in the build tree if NECKO_DEV_ENABLE_DISK_CACHE is set in the environment.  (I also got rid of some compiler warnings by changing some index variable to unsigned ints).

I tested the memory cache, and it works as it should, at least when I remember to turn on caching in my about:config :)
Attachment #371715 - Flags: superreview?(cbiesinger)
Attachment #371715 - Flags: review?(cbiesinger)
Comment on attachment 371715 [details] [diff] [review]
New fix

+        // use file cache in build tree only if asked, to avoid cache dir litter

it's the current directory... doesn't have to be the build tree
Attachment #371715 - Flags: superreview?(cbiesinger)
Attachment #371715 - Flags: superreview+
Attachment #371715 - Flags: review?(cbiesinger)
Attachment #371715 - Flags: review+
Attachment #369337 - Flags: superreview?(cbiesinger)
Attachment #369337 - Flags: review?(cbiesinger)
Attachment #369337 - Flags: review-
(In reply to comment #5)
> I tested the memory cache, and it works as it should, at least when I remember
> to turn on caching in my about:config :)

That'd certainly explain caching problems :)
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/73e08f744e9a
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.