Hi, This is intentional, as the calling method would return early if the htx is null (this is assuming that smp_prefetch_htx will not return a different result the second time?): htx = smp_prefetch_htx(smp, chn, 1); if (!htx) return 0; Rather than doing the null check again, I will instead change the method to take the htx which has already been checked. This way coverity should not have an issue with it, we avoid having an extra null check, and we remove the second call to smp_prefetch_htx which was unnecessary.
I will submit this as a separate patch to the other change. Thanks, Ben Shillito Developer [51Degrees]<https://51degrees.com/> O: +44 1183 287152<callto:441183287152> E: b...@51degrees.com<mailto:b...@51degrees.com?subject=Your%20Email> [https://51degrees.com/portals/0/images/twitterbird.png] @51Degrees<http://twitter.com/51Degreesmobi> [https://51degrees.com/portals/0/images/linkedinicon.png] 51Degrees<https://www.linkedin.com/company/2171864> [Find out More]<https://51degrees.com/emailsig.aspx> From: Илья Шипицин [mailto:chipits...@gmail.com] Sent: 24 October 2019 08:04 To: Ben Shillito <b...@51degrees.com> Cc: Willy Tarreau <w...@1wt.eu>; HAProxy <haproxy@formilux.org> Subject: Re: Possible optimization to 51d in multithread Hello, Ben. since you are going to touch 51d code, can you please review the following coverity finding (it thinks there might be null pointer dereference) ? 239 // No need to null check as this has already been carried out in the 240 // calling method 2. returned_null: smp_prefetch_htx returns NULL (checked 35 out of 36 times). [show details<https://scan6.coverity.com/eventId=28590715-1&modelId=28590715-0&fileInstanceId=94351345&filePath=%2Fsrc%2Fhttp_fetch.c&fileStart=172&fileEnd=294>] 3. var_assigned: Assigning: htx = NULL return value from smp_prefetch_htx. 241 htx = smp_prefetch_htx(smp, chn, 1); 242 4. Condition i < global_51degrees.header_count, taking true branch. 243 for (i = 0; i < global_51degrees.header_count; i++) { 244 name.ptr = (global_51degrees.header_names + i)->area; 245 name.len = (global_51degrees.header_names + i)->data; 246 ctx.blk = NULL; 247 CID 1403847 (#1 of 1): Dereference null return value (NULL_RETURNS)5. dereference: Dereferencing a pointer that might be NULL htx when calling http_find_header. [show details<https://scan6.coverity.com/eventId=28590715-16&modelId=28590715-1&fileInstanceId=94351414&filePath=%2Fsrc%2Fhttp_htx.c&fileStart=54&fileEnd=129>] 248 if (http_find_header(htx, name, &ctx, 1)) { 249 ws->importantHeaders[ws->importantHeadersCount].header = ws->dataSet->httpHeaders + i; 250 ws->importantHeaders[ws->importantHeadersCount].headerValue = ctx.value.ptr; 251 ws->importantHeaders[ws->importantHeadersCount].headerValueLength = ctx.value.len; 252 ws->importantHeadersCount++; 253 } 254 } 255} 256#endif ср, 23 окт. 2019 г. в 14:40, Ben Shillito <b...@51degrees.com<mailto:b...@51degrees.com>>: Hi Willy, Thanks for letting me know. I will get the work for this scheduled in and get a patch over to you. I agree it looks like a fairly small change for what I'm sure is a considerable performance increase for large systems. Good to see this level of attention to detail when it comes to the performance of HAProxy. Thanks, Ben Shillito Developer O: +44 1183 287152 E: b...@51degrees.com<mailto:b...@51degrees.com> T: @51Degrees -----Original Message----- From: Willy Tarreau [mailto:w...@1wt.eu<mailto:w...@1wt.eu>] Sent: 23 October 2019 07:04 To: Ben Shillito <b...@51degrees.com<mailto:b...@51degrees.com>> Cc: HAProxy <haproxy@formilux.org<mailto:haproxy@formilux.org>> Subject: Possible optimization to 51d in multithread Hi Ben, after Brian reported the thread performance regression affecting the pattern matchings in haproxy when relying on the LRU cache, I had a look at other users of the LRU cache and found that 51d.c is using it with a lock as well and may also suffer from a lack of linearity with threads. You may want to have a look at this patch I just committed to make the pattern LRU cache per thread : 403bfbb130 ("BUG/MEDIUM: pattern: make the pattern LRU cache thread-local and lockless") It's quite straightforward, and if you want to do the same on 51d.c, you just have to make your lru_tree pointer thread_local and allocate it for each thread in a small callback registered to be called after threads are initialized. Same for the call to lru64_destroy(). Then you can remove the lru lock and gain a lot of scalability. I'll let you have a look because I'd rather not break somehing non- obvious in your code and also because you know better than me how to benchmark the changes using the real lib and database, but if you need some help, just let me know. Given that it's quite small and simple a change, I'm fine with merging such a patch for 2.1 even a bit late (but only this, no other changes that are not bugfixes please). Willy This email and any attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender immediately and do not disclose, use, store or copy the information contained herein. This is an email from 51Degrees.mobi Limited, 5 Charlotte Close, Reading. RG47BY. T: +44 118 328 7152; E: i...@51degrees.com<mailto:i...@51degrees.com>; 51Degrees.mobi Limited t/as 51Degrees. This email and any attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender immediately and do not disclose, use, store or copy the information contained herein. This is an email from 51Degrees.mobi Limited, 5 Charlotte Close, Reading. RG47BY. T: +44 118 328 7152; E: i...@51degrees.com; 51Degrees.mobi Limited t/as 51Degrees.