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: [email protected]<mailto:[email protected]?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:[email protected]]
Sent: 24 October 2019 08:04
To: Ben Shillito <[email protected]>
Cc: Willy Tarreau <[email protected]>; HAProxy <[email protected]>
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
<[email protected]<mailto:[email protected]>>:
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: [email protected]<mailto:[email protected]>
T: @51Degrees
-----Original Message-----
From: Willy Tarreau [mailto:[email protected]<mailto:[email protected]>]
Sent: 23 October 2019 07:04
To: Ben Shillito <[email protected]<mailto:[email protected]>>
Cc: HAProxy <[email protected]<mailto:[email protected]>>
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: [email protected]<mailto:[email protected]>; 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: [email protected]; 51Degrees.mobi Limited t/as 51Degrees.
