[squid-dev] [PATCH] Case-insensitive URI schemes

[Alex Rousskov]

On 02/02/2017 05:53 AM, Eduard Bagdasaryan wrote:
> I applied your polishing suggestions to my latest patch
> re-attached it.

> +std::vector<SBuf> AnyP::UriScheme::LowercaseSchemeNames;

> +    static std::vector<SBuf> LowercaseSchemeNames;

We should avoid this code duplication:

  typedef std::vector<SBuf> LowercaseSchemeNames;
  static LowercaseSchemeNames LowercaseSchemeNames_;

but this is not why I am writing this email.


>          storeFsInit();      /* required for config parsing */
>          Fs::Init();
>          DiskIOModule::SetupAllModules();
>          StoreFileSystem::SetupAllFs();
>          Store::Init();
>          Auth::Init();      /* required for config parsing. NOP if !USE_AUTH */
>          Ip::ProbeTransport(); // determine IPv4 or IPv6 capabilities before parsing.
>          Format::Token::Init(); // XXX: temporary. Use a runners registry of pre-parse runners instead.
> +        AnyP::UriScheme::Init();

The Init() approach may seem like an obvious no-cost optimization, but
it actually has serious negative side effects, including the necessity
to answer the "When to call Init()?" question. In this particular case,
we must call Init()

* after SBuf is operational and
* before anybody might create something containing a UriScheme.

The first part is fairly easy IIRC -- SBuf should already be working at
static initialization time and working efficiently after Mem::Init().

The second one is less obvious. Nobody knows for sure what exactly all
those Init() calls quoted above do now and especially what they will do
tomorrow. Is it possible that one of them will create a URI and will
want to use a UriScheme for that? I think it is! Since UriScheme does
not need any of the above modules (AFAICT), IMO it should be initialized
first, before storeFsInit().

Since Amos did not ask you to go back to as-needed initialization, I
will not ask for that either. However, please check whether we can move
the AnyP::UriScheme::Init() call up, to place it above storeFsInit().


Both of the above changes can be done during commit.


Thank you,

Alex.



> On 02.02.2017 11:33, Amos Jeffries wrote:
>> On 1/02/2017 2:18 a.m., Eduard Bagdasaryan wrote:
>>> Optimized with static array as you suggested and
>>> re-attached the patch.
>>>
>> Thank you, looks like this one removes most of the performance
>> regression.
>>
>> Just some polishing for src/anyp/UriScheme.cc ;
>>
>>
>> in AnyP::UriScheme::UriScheme
>>
>> * this TODO seems reasonable and simple enough, please do it:
>>
>>   +    // the caller knows nothing about the scheme
>> +    // TODO: Avoid special casing by storing this string in the
>> generated ProtocolType_str?
>>       else if (theScheme_ == AnyP::PROTO_UNKNOWN)
>> -        // image could be actually unknown and not provided
>>           image_ = "(unknown)";
>>
>>
>> in AnyP::UriScheme::LowercaseScheme:
>>
>> * please use the Squid coding style of parameters being on a line before
>> the function name.
>>
>> * please do use emplace_back instead of push_back. Simple as it is the
>> SBuf is not a pointer.
>>
>> * please add a TODO note about making the ProtocolType enum use
>> base/EnumIterator.h instead of an int for-loop.
>>
>>
>> +1. I don't think this needs another review, just the polish.
>>
>> Amos
>>
> 
> 
> _______________________________________________
> squid-dev mailing list
> squid-dev at lists.squid-cache.org
> http://lists.squid-cache.org/listinfo/squid-dev
>