Re: [MERGE] branch prediction hints
2010/10/13 Henrik Nordström hen...@henriknordstrom.net: ons 2010-10-13 klockan 20:50 +0200 skrev Kinkie: I agree. That's why I propose to only use it - if we do use it for anything - for singletons, where it should be a no-brainer. Have you checked if it makes any difference in the proposed use you see today? Yes it does. The resulting code is more compact, and has a different jump structure. Unfortunately I am not an assembler expert to judge whether it is a good or a bad thing; if you wish I can attach the asm code of the relevant function; in general I'd tend to trust the gcc authors, and the fact that it's a technique widely used in a highly sensitive project (the Linux kernel) [http://kerneltrap.org/node/4705]. IF we decide it's worth to use this, other obvious candeidates for useage would be Must() and assert (the latter case may be already done by the library for us). As I may have already stated, I would advise against using this in regular code. -- /kinkie
Re: Client bandwidth limits for squid3-trunk
On 10/14/2010 02:35 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 23:09:43 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: Hi all, I am working on adding support for client-side bandwith limiting on squid3. The related description of this feature is at http://wiki.squid-cache.org/Features/ClientBandwidthLimit The feature implemented by Alex Rousskov for his 3p1-rock branch. In this mail I am including a first port for squid-trunk. The patch needs more testing and probably farther development but I am posting the current patch for preview and initial feedback. A short description included in the patch. Regards, Christos Can't see anything obvious in the code. :) I know :-) I am still trying to clarify some parts of the code. Maybe there are comments for the configuration parameters, or other comments about how good policy is to use the comm handlers (commHandleWrite function) etc... Can you at least make the new functions into members where possible. In this case clientdbSetWriteLimiter into ClientInfo::setWriteLimiter. OK. In hunk @@ -3125,40 +3129,82 @@ it looks like ClientDelayPools pools(Config.ClientDelay.pools); could be reduced in scope to inside the if(). +1 from me. Whats the plan for committing this? Currently I am running some basic tests using 2-3 clients.. Probably it will be ready next week. Regards, Christos Amos
Re: [PATCH] log error details
On 14/10/10 22:04, Tsantilas Christos wrote: On 10/14/2010 12:58 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 16:25:34 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: . * Can FileNameHashCached() be made a private static method of TextException and work with the file and line details passed in to cnstructor please? Will not work Amos. Ah I overlooked the static local. FileNameHash() could still be a member, but protected with friend function FileNameHashCached(). The singleton hack making local copies in almost every file .o is likely to symbol clash on the more picky compilers like MacOSX. We need a copy of the FileNameHashCached in each .o file, which will compute and cache once the hash of its filename. Alternatively we can compute the hash when the Must exception called (direct use of the FileNameHash). IMO that would be better. A system throwing and failing a lot of Must() requirements has worse problems than a few cycles of hashing. Probably you are right here. In the other hand some thousands of transactions aborted the same time for the same reason, will cause squid to compute the same hash value some thousand times. If we decide to not cache the hash value and there is not any objection from others developers I will do the followings: - remove the FileNaneHashCached and the FileNameHashCached class. - make FilenameHash a private method of the TextException class. - Compute the hash only in TextException::id() method. This will compute the hash value only if it is requested, not for all Must() * NP: if performance is _that_ much of a problem we had best look into pre-calculating FileNameHash() results as a const parameter to the Must/TextcHere macros during build. I can not find any simple way for this one. Any hint? But if we decide to not cache the hash value of the file name we do not need it. A separate script that runs and does the filename hashing. It might be run by configure, top level Makefile, or if it was useful enough we could add it to source-maitenance.sh for permanent embedding. Easiest would be for it to add/update a #define FILENAMEHASH abf0 at the top of each file which is used by the Must() macro later on. If the hash bits were split 16/16 then the hash need only be two bytes and easily identified by us humans as the first two of the EXCEPTION id. The __LINE__ bit would still need to be masked on later to avoid problems with patchings. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
Re: Client bandwidth limits for squid3-trunk
On 14/10/10 22:32, Tsantilas Christos wrote: On 10/14/2010 02:35 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 23:09:43 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: Hi all, I am working on adding support for client-side bandwith limiting on squid3. The related description of this feature is at http://wiki.squid-cache.org/Features/ClientBandwidthLimit The feature implemented by Alex Rousskov for his 3p1-rock branch. In this mail I am including a first port for squid-trunk. The patch needs more testing and probably farther development but I am posting the current patch for preview and initial feedback. A short description included in the patch. Regards, Christos Can't see anything obvious in the code. :) I know :-) I am still trying to clarify some parts of the code. Maybe there are comments for the configuration parameters, or other comments about how good policy is to use the comm handlers (commHandleWrite function) etc... Can you at least make the new functions into members where possible. In this case clientdbSetWriteLimiter into ClientInfo::setWriteLimiter. OK. In hunk @@ -3125,40 +3129,82 @@ it looks like ClientDelayPools pools(Config.ClientDelay.pools); could be reduced in scope to inside the if(). +1 from me. Whats the plan for committing this? Currently I am running some basic tests using 2-3 clients.. Probably it will be ready next week. Thanks. Okay then depending on Alex we may directly clash between the is and the Comm::Io::Write polishing. I'm unsure how the commHandleWrite function will fit into that layout. There is a matching read delay system we will have to match it to somehow. We currently have the same queue pattern in three places under three names: AcceptLimiter for incoming connection acceptances, read Delays for delay pools, and now write Quota for client pools. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
Re: [PATCH] log error details
On 10/14/2010 12:46 PM, Amos Jeffries wrote: On 14/10/10 22:04, Tsantilas Christos wrote: On 10/14/2010 12:58 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 16:25:34 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: . * Can FileNameHashCached() be made a private static method of TextException and work with the file and line details passed in to cnstructor please? Will not work Amos. Ah I overlooked the static local. FileNameHash() could still be a member, but protected with friend function FileNameHashCached(). The singleton hack making local copies in almost every file .o is likely to symbol clash on the more picky compilers like MacOSX. We need a copy of the FileNameHashCached in each .o file, which will compute and cache once the hash of its filename. Alternatively we can compute the hash when the Must exception called (direct use of the FileNameHash). IMO that would be better. A system throwing and failing a lot of Must() requirements has worse problems than a few cycles of hashing. Probably you are right here. In the other hand some thousands of transactions aborted the same time for the same reason, will cause squid to compute the same hash value some thousand times. If we decide to not cache the hash value and there is not any objection from others developers I will do the followings: - remove the FileNaneHashCached and the FileNameHashCached class. - make FilenameHash a private method of the TextException class. - Compute the hash only in TextException::id() method. This will compute the hash value only if it is requested, not for all Must() * NP: if performance is _that_ much of a problem we had best look into pre-calculating FileNameHash() results as a const parameter to the Must/TextcHere macros during build. I can not find any simple way for this one. Any hint? But if we decide to not cache the hash value of the file name we do not need it. A separate script that runs and does the filename hashing. It might be run by configure, top level Makefile, or if it was useful enough we could add it to source-maitenance.sh for permanent embedding. Easiest would be for it to add/update a #define FILENAMEHASH abf0 at the top of each file which is used by the Must() macro later on. If the hash bits were split 16/16 then the hash need only be two bytes and easily identified by us humans as the first two of the EXCEPTION id. The __LINE__ bit would still need to be masked on later to avoid problems with patchings. It will not be so easy. We should add a #undef FILENAMEHASH/#define FILENAMEHASH after the last #include in each file. Also should be added in *.h files too, because a Must may used in include files. The current solution looks much simpler and better... Also using a 16bit hash can produce the same hash value for two files, but it is not a real problem because it is unusual to have two files with the same hash value and a Must expression in the same line. Amos
Re: [PATCH] SMP Cache Manager, Phase2
On 14/10/10 18:09, Alex Rousskov wrote: SMP Cache Manager, Phase2 implementation. snip More polishing left for future projects: Move CacheManager to Mgr namespace and src/mgr/ directory. Use SBuf instead of String for ActionParams and TypedMsgHdr. Rename Ipc::TypedMsgHdr to Ipc::Msg, Ipc::SocketMsg, or similar because it maintains more than just msghdr struct. Fix #include order in old source files affected by the Mgr::RegisterAction change. More action aggregation and Phase3 changes. Thank you. Audit Checks: * please make sure all the new mgr/*.h in the include lists are not adding regressions. There seem to be a fair few added which are merely mussing up otherwise find lists. * Why double across the board for all the counters? does it accurately portray TB counts to the byte for large boxes? * don't forget to update SourceLayout with the src/mgr/ progress :) Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
The order of #include statements
Hello, We require that #include statements are alphabetized, with certain exceptions. The presence of old files with out-of-order #includes makes it difficult to enforce this policy because it is often difficult to say whether the #includes were already 100% ordered without ordering them first and then comparing the results -- a time consuming process. Also, it is not clear what the alphabetical order is, exactly, when files contain non-alphabet characters such as '.' and '_'. The natural order may differ from what a sort(1) command would produce, for example. There is also a question of case-sensitivity. Finally, it is not clear how conditional #include statements should be handled, especially when there are multiple #includes under a single #ifdef condition. I suggest that we automate this instead of doing everything manually. It should not be very difficult: 1. #include goes before any #include . 2. do not touch the inner #include order. Move includes carefully as they may have #ifdef protections. It is probably easier and safer to move includes only. No includes should be included conditionally (this will require a few code changes). 3. each src/ file, with a few hard-coded exceptions, must start with #include . If there is no to include, include config.h. Do not include config.h or squid.h if there is another include. 4. config.h, if any, goes first. squid.h, if any, goes first or second, depending on whether there is config.h 5. Other includes are sorted using ascending ASCII character value order. Small letters are capitalized for sorting purposes. I am sure there will be a few exceptions and the above needs some polishing, but I think we can automate this pretty well and add to the SourceFormating scripts. Is there a better way to enforce the #include rules? Or should we drop the ordering rules instead? Thank you, Alex.
Re: [PATCH] SMP Cache Manager, Phase2
On 10/14/2010 07:20 AM, Amos Jeffries wrote: On 14/10/10 18:09, Alex Rousskov wrote: SMP Cache Manager, Phase2 implementation. snip More polishing left for future projects: Move CacheManager to Mgr namespace and src/mgr/ directory. Use SBuf instead of String for ActionParams and TypedMsgHdr. Rename Ipc::TypedMsgHdr to Ipc::Msg, Ipc::SocketMsg, or similar because it maintains more than just msghdr struct. Fix #include order in old source files affected by the Mgr::RegisterAction change. More action aggregation and Phase3 changes. Thank you. Audit Checks: * please make sure all the new mgr/*.h in the include lists are not adding regressions. There seem to be a fair few added which are merely mussing up otherwise find lists. I assume you are talking about the alphabetical order of old #include directives. I will fix all the affected old files as a separate commit. The newly added files should have the right order of includes already. * Why double across the board for all the counters? I am not sure double was the right choice, but here is the rationale: - 32bit counters used in the old code cannot be used with SMP because they are likely to overflow as we merge results from multiple kids. - Even some 64bit counters (e.g., those counting small objects like bytes) might overflow when many kids are merged. - Using two 64bit counters (gb_*) seems like an overkill here. - Using double counters is simple and will work forever. Many of the counters are not reported raw but as rates or fractions anyway. We should have used a typedef for this. Sigh. does it accurately portray TB counts to the byte for large boxes? If we use doubles, I do _not_ think huge values will be accurate to the byte. Is it really important for something like a 3 terabyte value to be accurate to the last byte? If it is important, what do you suggest? 64bit counters? A pair of 64bit counters? A custom decision for every counter?? * don't forget to update SourceLayout with the src/mgr/ progress :) Done. Thank you, Alex.
Re: [PATCH] log error details
On 10/13/2010 07:25 AM, Tsantilas Christos wrote: You might even drop that down to EXCEPTION_a10b for size reducing. Maybe it is better to use a constant form for the err_detail formating code: DESCR[=anID] It will be simpler for log analysers, but I have not strong opinion. Should we change the SYSERR=errorcode to a SYSERR_errorcode format too? I think that it is better to use '=' as a separator everywhere we log additional error details. We may add lists of such codes (Amos' request from the first patch review) and other additional info later, making '_' look increasingly inappropriate. If some log processing tool drops everything after '=', it will still work correctly but will miss some of the information because it does not know about our new features. That is fine and might even be desirable as we extend details, I think. Thank you, Alex.
Re: [PATCH] log error details
On 10/14/2010 04:28 AM, Tsantilas Christos wrote: On 10/14/2010 12:46 PM, Amos Jeffries wrote: On 14/10/10 22:04, Tsantilas Christos wrote: On 10/14/2010 12:58 AM, Amos Jeffries wrote: Plan A: the posted patch (compute runtime, roughly once per file). Plan B: compute runtime in TextException, all the time. Plan C: hard-coded constant hash value in every .h and .cc file. The singleton hack making local copies in almost every file .o is likely to symbol clash on the more picky compilers like MacOSX. Possible, but not very likely because it would be a C++ standard violation and because we probably already have identical static symbols in many .o files. We can test though and revert to Plan B or C if there are indeed clashes. We need a copy of the FileNameHashCached in each .o file, which will compute and cache once the hash of its filename. Alternatively we can compute the hash when the Must exception called (direct use of the FileNameHash). IMO that would be better. A system throwing and failing a lot of Must() requirements has worse problems than a few cycles of hashing. Probably you are right here. In the other hand some thousands of transactions aborted the same time for the same reason, will cause squid to compute the same hash value some thousand times. If we decide to not cache the hash value and there is not any objection from others developers I will do the followings: - remove the FileNaneHashCached and the FileNameHashCached class. - make FilenameHash a private method of the TextException class. - Compute the hash only in TextException::id() method. This will compute the hash value only if it is requested, not for all Must() Let's call this Plan B. * NP: if performance is _that_ much of a problem we had best look into pre-calculating FileNameHash() results as a const parameter to the Must/TextcHere macros during build. I can not find any simple way for this one. Any hint? But if we decide to not cache the hash value of the file name we do not need it. A separate script that runs and does the filename hashing. It might be run by configure, top level Makefile, or if it was useful enough we could add it to source-maitenance.sh for permanent embedding. This is Plan C. I do not think configure and similar build-time tools would work well here. We do not want to work with foo.cc.in files all the time :-). I agree that hard-coded hashes can be automated as a part of SourceFormat scripts. The biggest drawback of this approach is that it requires wrapping every header file in push/redefine/pop macros (or at least redefine macros after all the #include lines). Nevertheless it is doable. Also using a 16bit hash can produce the same hash value for two files, but it is not a real problem because it is unusual to have two files with the same hash value and a Must expression in the same line. If the hash is automated and hard-coded by a script, the script can even make sure there are no duplicates, I think. I do not have a strong preference, but if nobody has a strong preference, then I would recommend this order of commits/backtracking if we find actual problems: Plan A -- because it is already implemented. Plan B -- because it is the simplest. Plan C -- because it is fast and eliminates dupes. HTH, Alex.
Re: [MERGE] Branch prediction hints
On 10/13/2010 10:14 AM, Francesco Chemolli wrote: Hi all, This patch supersedes yesterday's; it impements autoconf-based detection of compiler builtins. The purpose of this feature is to give explicit hints to compilers supporting them about the likely outcome of an if() clause; thus allowing the compiler to do explicit optimizations (CPU predictor hinting, code layout optimizations) based on the predicted outcome. The obvious case is the singleton pattern, where the existence check will always be true except for the first time. +#define likely(x) __builtin_expect(!!(x), 1) +#define unlikely(x) __builtin_expect(!!(x), 0) * Global names should start with a capital letter. * Check that the new names were not already #defined, just in case. * The new macros should be used in performance-critical parts of the code or in frequently used parts. The cache manager singleton optimization provided as an illustration is neither, and so it is an example of how _not_ to use the macros. If you need an example, consider optimizing Must and Assert instead. * This patch should not be committed unless somebody promises to follow up with changes to use the new macros. I vote -0 on this because I expect that using this code will result in - no measurable performance improvement for a long time; - some overheads related to explaining and maintaining the macros. I will change my vote to +1 if somebody demonstrates measurable performance improvement on a simple Polygraph test or deployed Squid after using the proposed macros inside Must() and Assert() definitions. Thank you, Alex.
Re: debugging Squid ICAP interface
On 10/12/2010 11:51 AM, Marcus Kool wrote: I also have seen Squid behaving unexpectedly (2 minutes timeouts where it seems not handle any request from a browser, assertion failure). If you can reproduce this, please file a bug report with details, especially if this happens to a recent Squid version. I have various observations and questions about the Squid ICAP interface and like to discuss these with the persons who wrote or know much about the ICAP client part of Squid. I like to know with whom I can discuss this and which mailing list to use. As Henrik said, you came to the right place. Cheers, Alex.
Re: Client bandwidth limits for squid3-trunk
On 10/14/2010 04:19 AM, Amos Jeffries wrote: On 14/10/10 22:32, Tsantilas Christos wrote: On 10/14/2010 02:35 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 23:09:43 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: Hi all, I am working on adding support for client-side bandwith limiting on squid3. The related description of this feature is at http://wiki.squid-cache.org/Features/ClientBandwidthLimit The feature implemented by Alex Rousskov for his 3p1-rock branch. In this mail I am including a first port for squid-trunk. The patch needs more testing and probably farther development but I am posting the current patch for preview and initial feedback. A short description included in the patch. Regards, Christos Can't see anything obvious in the code. :) Do you mean the code is so bad you cannot understand anything or so good that you cannot find any problems? I think Christos assumed the former. :-) I know :-) I am still trying to clarify some parts of the code. It looks like most of the code is well-commented and the patch preamble describes the overall architecture. The critical parts are in queuing/delaying writes in Comm, without those delays being visible to the Comm callers, of course. I would be happy to help with clarifying the code, but I cannot do that without specific questions. IIRC, I did not write the original version, but I rewrote most of it, so I should be able to grok what the code does. Can you at least make the new functions into members where possible. In this case clientdbSetWriteLimiter into ClientInfo::setWriteLimiter. I am not against that (it is just one function, albeit with conditional compilation), but I think we should not do it because it is outside this project scope: ClientInfo has no methods now, and all current manipulation code is written as functions. We can and should change that, of course, but * _properly_ doing that is not what this project is about and will result in many totally irrelevant changes. * adding just one conditionally-compiled method when everything else is done via functions will make the code look less consistent and less clear. Either way is fine with me though. In hunk @@ -3125,40 +3129,82 @@ it looks like ClientDelayPools pools(Config.ClientDelay.pools); could be reduced in scope to inside the if(). Yes, it should be. Also many comments should use Doxygen style. I think this code was written before we started encouraging Doxygen use... depending on Alex we may directly clash between the is and the Comm::Io::Write polishing. I'm unsure how the commHandleWrite function will fit into that layout. What is Comm::Io::Write polishing layout? Is it the comm write and commfd_table cleanup patch posted on 2010/10/10? There is a matching read delay system we will have to match it to somehow. We currently have the same queue pattern in three places under three names: AcceptLimiter for incoming connection acceptances, read Delays for delay pools, and now write Quota for client pools. Indeed. However, all three queues are different in nature and are used in different places so there is no significant code duplication. Thank you, Alex.
[PATCH] InstanceId formatting
Hi all,
this patch changes slightly the formatting of InstanceId's output so
that it's easier to parse and isolate.
Opinions?
=== modified file 'src/base/InstanceId.h'
--- src/base/InstanceId.h 2010-10-04 14:49:57 +
+++ src/base/InstanceId.h 2010-10-14 22:33:00 +
@@ -43,7 +43,7 @@
#define InstanceIdDefinitions(Class, prefix) \
template std::ostream \
InstanceIdClass::print(std::ostream os) const { \
-return os Prefix value; \
+return os '[' Prefix '#' value ']'; \
} \
template const char *InstanceIdClass::Prefix = prefix; \
template InstanceIdClass::Value InstanceIdClass::Last = 0
--
/kinkie
Re: [PATCH] Subscription
On 10/07/2010 01:54 AM, Amos Jeffries wrote: On 06/10/10 18:15, Alex Rousskov wrote: You may also want to add something like: \todo Add a way for Subscriber to cancel the subscription and for Publisher test for subscription cancellation? This is already provided by RefCount API inherited to Subscription::Pointer. Yes, if we do not need to notify Publisher, then the underlying bits are kind of already there, but the Publisher still needs a clean way to test for cancellation (a new Subscription method, I presume) and the Subscriber needs a way to cleanly cancel the subscription (a new Subscription method, I presume) along with associated documentation. The Subscription::Pointer can be set to NULL for cancellation/erasure, set to point at another subscription, and tested for NULL. The interesting case here is Subscribers canceling subscriptions. The pointer to Subscription stored in Publisher is not accessible to Subscribers. An API extension (new Subscription methods) is needed. The original Call pointed to is never scheduled and can be deleted without canceling right?. Yes, but the API does not have a notion of the original Call. You are thinking of a specific API implementation called CallSubscription. I am talking about the API itself. Any Calls spawned are outside the scope of the subscription itself once they leave the callback() method. True, although user may need a reminder about that caveat. + virtual AsyncCall::Pointer callback() = 0; Please document whether callback() can return a nil pointer and, if yes, what would that mean to the Publisher. I suggest that it must not return nil. It could be. Depends on the inheriting childs implementation. My point is that it either it should be prohibited (for any implementations) or allowed. I suggest that it is prohibited until we support cancellations. If it is allowed now, you need to document what Publisher is supposed to do when callback() returns nil. Cheers, Alex.
Re: The order of #include statements
On 15/10/10 06:09, Alex Rousskov wrote: Hello, We require that #include statements are alphabetized, with certain exceptions. The presence of old files with out-of-order #includes makes it difficult to enforce this policy because it is often difficult to say whether the #includes were already 100% ordered without ordering them first and then comparing the results -- a time consuming process. Also, it is not clear what the alphabetical order is, exactly, when files contain non-alphabet characters such as '.' and '_'. The natural order may differ from what a sort(1) command would produce, for example. There is also a question of case-sensitivity. Finally, it is not clear how conditional #include statements should be handled, especially when there are multiple #includes under a single #ifdef condition. I suggest that we automate this instead of doing everything manually. It should not be very difficult: 1. #include goes before any #include . 2. do not touch the inner #include order. Move includes carefully as they may have #ifdef protections. It is probably easier and safer to move includes only. No includes should be included conditionally (this will require a few code changes). The current policy (which has not been held to) is to wrap all includes and test for them in configure.in. This could work both for and against automation. The pro being that we can shuffle at will outside of compat/ and enforce the wrapping #if. Along with compat/os/* performing undef of the wrappers after doing any exception includes. 3. each src/ file, with a few hard-coded exceptions, must start with #include . If there is no to include, include config.h. Do not include config.h or squid.h if there is another include. Why the exception to not include config.h or squid.h? 4. config.h, if any, goes first. squid.h, if any, goes first or second, depending on whether there is config.h squid.h requires config.h internally first. If squid.h is included it overrides config.h being needed. The goal with these two was to include config.h first outside of src/ and squid.h first inside of src/. Currently hampered by the long list of dependencies added to the unit tests by using squid.h. The earlier suggestion to move squid.h to squid_old.h and shuffle stuff into a cleaner squid.h for src/ may have to be taken first. 5. Other includes are sorted using ascending ASCII character value order. Small letters are capitalized for sorting purposes. I don't think we need that capitilization clause. Enforcement and formatting is done centrally so we don't exactly have issues with portability on the comparisons. ASCII sorting is not a black-box process to any of us I hope. I am sure there will be a few exceptions and the above needs some polishing, but I think we can automate this pretty well and add to the SourceFormating scripts. Is there a better way to enforce the #include rules? Or should we drop the ordering rules instead? I'm for keeping them. It's a bit of a no-op on small include lists. But really matters on the long ones. The patch conflicts from adding a new include at end of the list are also mostly gone with this policy. The exceptions will be in compat/ and the third-party library sources which Henrik has suggested should all be under lib/. I have a patch ready to test that Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
Re: Client bandwidth limits for squid3-trunk
On 15/10/10 08:20, Alex Rousskov wrote: On 10/14/2010 04:19 AM, Amos Jeffries wrote: On 14/10/10 22:32, Tsantilas Christos wrote: On 10/14/2010 02:35 AM, Amos Jeffries wrote: On Wed, 13 Oct 2010 23:09:43 +0300, Tsantilas Christos chtsa...@users.sourceforge.net wrote: Hi all, I am working on adding support for client-side bandwith limiting on squid3. The related description of this feature is at http://wiki.squid-cache.org/Features/ClientBandwidthLimit The feature implemented by Alex Rousskov for his 3p1-rock branch. In this mail I am including a first port for squid-trunk. The patch needs more testing and probably farther development but I am posting the current patch for preview and initial feedback. A short description included in the patch. Regards, Christos Can't see anything obvious in the code. :) Do you mean the code is so bad you cannot understand anything or so good that you cannot find any problems? I think Christos assumed the former. :-) Sorry Christos. I meant the latter (smiley face = happy with it?). I know :-) I am still trying to clarify some parts of the code. It looks like most of the code is well-commented and the patch preamble describes the overall architecture. The critical parts are in queuing/delaying writes in Comm, without those delays being visible to the Comm callers, of course. I would be happy to help with clarifying the code, but I cannot do that without specific questions. IIRC, I did not write the original version, but I rewrote most of it, so I should be able to grok what the code does. My only issues are with using a queue that can be replicated for the read and accept operations. A week ago I would have needed explanations but I spent a lot of time last weekend reading the comm_write structures. This all seems to fit relatively nicely. Albeit sans optimization. Can you at least make the new functions into members where possible. In this case clientdbSetWriteLimiter into ClientInfo::setWriteLimiter. I am not against that (it is just one function, albeit with conditional compilation), but I think we should not do it because it is outside this project scope: ClientInfo has no methods now, and all current manipulation code is written as functions. We can and should change that, of course, but * _properly_ doing that is not what this project is about and will result in many totally irrelevant changes. * adding just one conditionally-compiled method when everything else is done via functions will make the code look less consistent and less clear. Either way is fine with me though. The other functions are all the scope of my Comm::Io::Write cleanup patch. This function does not easily enter the writing scope. It's ClientInfo manipulation. In hunk @@ -3125,40 +3129,82 @@ it looks like ClientDelayPools pools(Config.ClientDelay.pools); could be reduced in scope to inside the if(). Yes, it should be. Also many comments should use Doxygen style. I think this code was written before we started encouraging Doxygen use... depending on Alex we may directly clash between the is and the Comm::Io::Write polishing. I'm unsure how the commHandleWrite function will fit into that layout. What is Comm::Io::Write polishing layout? Is it the comm write and commfd_table cleanup patch posted on 2010/10/10? Yes. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.8 Beta testers wanted for 3.2.0.2
