On 1/01/2014 8:40 a.m., Alex Rousskov wrote: > Hello, > > The attached patch contains initial Large Rock and Collapsed > Forwarding support, also available as a Launchpad branch: > lp:~measurement-factory/squid/collapsed-fwd/ > > Large Rock: Support disk (and shared memory) caching of responses > exceeding one db slot (or one shared memory page) in size. A single db > slot/page size is still limited to 32KB (smaller values can be > configured for disk caches using the newly added cache_dir slot-size > option). Removal of old rock cache dir (followed by squid-z) is required > -- the on-disk db structure has changed. > > Collapsed Forwarding: Optionally merge concurrent cachable requests for > the same URI earlier: After the request headers have been parsed (as > before), but now _before_ the response headers have been received. > Merging of requests received by different SMP workers is supported. > Controlled by the new collapsed_forwarding directive in squid.conf. > Disabled by default because all but one of the merged requests have to > be delayed (until the response headers are received) for the merging to > work, which may be worse than forwarding all concurrent requests > immediately. The overall feature idea and request eligibility conditions > are based on Collapsed Forwarding in Squid2. > > Here is a summary of other important changes (the branch log contains > the details and I propose that the branch is merged to preserve them): > > * Tightened StoreEntry locking. Split StoreEntry::lock() into "just > lock" and "update entry reference time" interfaces, addressing an old > XXX. Improved entry lock/unlock debugging. Needs more work. > > * Adjusted StoreIOState::write() API to allow callers detect write errors. > > * Simplified MemObject::write() API to remove an essentially unused > callback. > > * Mark client streams that sent everything as STREAM_COMPLETE. The old > code used STREAM_UNPLANNED_COMPLETE if the completed stream was > associated with a non-persistent connection, which did not make sense to > me and, IIRC, led to store entry aborts even though the entries were not > damaged in any way. > > * mem_hdr::hasContigousContentRange() now returns true for empty ranges. > > * Support "appending" ReadWriteLock state that can be shared by readers > and the writer. The writer promises not to update key metadata (except > growing object size and next pointers) and readers promise to be careful > when reading growing slices. > > * Fixed StoreEntry::mayStartSwapOut() logic to handle terminated swapouts. > > * Improved STORE_MEM_CLIENT detection and documented known (and mostly > old) StoreEntry::storeClientType() problems. > > * Removed StoreEntry::hidden_mem_obj hack. > > * Polished StoreEntry debugging to report more info, less noise. Use e: > prefix. > > * Added a script to extract store entry(ies) debugging from cache.log. > Needs more work. > > > The latest code passes built-in Squid test cases, some of its revisions > have been independently tested in at least two labs, and its earlier > incarnations have been successfully deployed in at least one production > environment. However, given their size and complexity, the proposed > changes are likely to introduce at least some instability and > regressions if accepted to trunk. We will need to monitor and address > those problems as needed. > > > This patch does not contain several large/important Store changes that > we still should make [soon], including: > > * Optimize rock db loading when Squid starts. > > * Support Vary caching in shared memory. > > * Fully support cached HTTP response header updates. > > * Stop using the Store class as a kitchen sink for Store-related APIs. > > * Better names and clear role separation for classes responsible for > memory and disk caches (where "disk cache" is "all cache_dirs taken > together and treated as a single cache"). > > * Migration of store_table global into something that better integrates > with SMP needs (probably similar to the proposed Transients but without > SMP overheads). > > * Refcounting of StoreEntries. > > > Given the current demand for Large Rock and Collapsed Forwarding > changes, combined with the amount of time it will take to implement the > missing changes listed above, and the size/complexity of the already > implemented changes, I think it is better to merge the implemented > changes now than wait for more changes to come. > > An earlier version of this code was posted to squid-dev as a PREVIEW a > few months ago. Amos kindly reviewed that posting. My response to his > review is below. >
Thank you for addressing those earlier comments. I've taken a stab at reading through this massive patch again. But too much of it is in areas of code I dont already know well enough to identify bugs, and as usual you dont leave any simple issues to point at :-) I did spot: in src/tests/stub_CollapsedForwarding.cc: * please remove the copyright blurb. Did Robert really design that stub for you? +1. IMHO, if nobody else wants to take a stab at a full audit I think you should just merge this and we fix the bugs as found. Amos
