Re: [PREVIEW] SMP caching, part1: Core changes and addition of shared memory cache

 On Sun, 15 May 2011 15:11:08 -0600, Alex Rousskov wrote:
> Hello,
>
> The attached patch contains the first part of the SMP caching
> changes described in the "SMP Rock Store and shared memory cache
> patches" email. I labeled it [PREVIEW] because of the following
> outstanding TODO items:
>
> * Portability tests and changes (via Hudson).
> * Add mem_cache shared=on|off parameter.
>
> However, I do not expect the above items to cause significant changes
> to
> the patch.
>
> The patch preamble contains a detailed change log.
>
> For the complete set of changes (all three parts), and for testing,
> please see the following Launchpad branch:
> lp:~measurement-factory/squid/3p2-rock
>
>
> Thank you,
>
> Alex.

 Just done a quick read-through...

 includes/Array.h
  - can go in separately and immediately if you wish.

 src/adaptation/icap/*
  - changes appear to be relatively separate. I suspect that RST
 handling may be of additional use with some 3.1 error recovery. If you
 agree that can go in separately and let me know to port it back for use
 there.

 src/cf.data.pre:
  - you are adding documentation for cache_dir "rock" type. That should
 be in the other patch where rock is actually added.

  - contains documentation for logformat width changes. Please remove
 from this patch. AFAIK the width changes are still waiting on a separate
 commit to happen. If that was already committed, please point me at the
 trunk revno and apply this extra documentation fix separately so I can
 group it with the relevant feature patch set.

  - contains documentation from "Dynamic adaptation plan over multiple
 vectoring points".
    Please remove and apply separately if still relevant, so it too can
 be grouped properly to its feature patch set.

 src/base/RunnersRegistry.*:
  - what are runners? please document a bit clearer than "different
 modules".

 src/store_rebuild.cc:
  - Please convert the debugs() level-1 to DBG_IMPORTANT while moving
 the code to storeRebuildLoadEntry() and storeRebuildParseEntry().
    (search for ", 1," to find other places in the patch needing same)

  - Also instead of using HERE they need "WARNING:" prefix and a suffix
 statement that Squid will erase the broken or collision object from
 cache.

 src/main.cc:
  - chunk line 1232 has similar debugs() issues missing DBG_CRITICAL and
 "FATAL:" prefix in the new version added.

 In StoreEntry::swapoutPossible() the test for "if (expectedEnd < 0)"
 looks out of place. IMO the current-min > global-max should go above it
 to short-cut very large unknown-length objects repeating the swapout
 tests when they grow bigger than cacheable. Please confirm that.

 Amos
Received on Mon May 16 2011 - 05:16:16 MDT