Implementation of net::HttpUtil::IsToken inefficient |
|||||
Issue descriptionclang compiles the IsTokenChar() part of IsToken() as follows: 24d62fd: 84 db test %bl,%bl 24d62ff: 41 0f 99 c1 setns %r9b 24d6303: 80 fb 1f cmp $0x1f,%bl 24d6306: 0f 97 44 24 ef seta -0x11(%rsp) 24d630b: 80 fb 7f cmp $0x7f,%bl 24d630e: 41 0f 95 c2 setne %r10b 24d6312: 89 d8 mov %ebx,%eax 24d6314: 0c 01 or $0x1,%al 24d6316: 3c 29 cmp $0x29,%al 24d6318: 41 0f 95 c0 setne %r8b 24d631c: 41 89 db mov %ebx,%r11d 24d631f: 41 80 cb 02 or $0x2,%r11b 24d6323: 41 80 fb 3e cmp $0x3e,%r11b 24d6327: 41 0f 95 c6 setne %r14b 24d632b: 80 fb 40 cmp $0x40,%bl 24d632e: 0f 95 c1 setne %cl 24d6331: 80 fb 2c cmp $0x2c,%bl 24d6334: 41 0f 95 c7 setne %r15b 24d6338: 3c 3b cmp $0x3b,%al 24d633a: 41 0f 95 c4 setne %r12b 24d633e: 80 fb 5c cmp $0x5c,%bl 24d6341: 41 0f 95 c5 setne %r13b 24d6345: 80 fb 22 cmp $0x22,%bl 24d6348: 0f 95 c2 setne %dl 24d634b: 80 fb 2f cmp $0x2f,%bl 24d634e: 40 0f 95 c6 setne %sil 24d6352: 80 fb 5b cmp $0x5b,%bl 24d6355: 40 0f 95 c7 setne %dil 24d6359: 80 fb 5d cmp $0x5d,%bl 24d635c: 40 0f 95 c5 setne %bpl 24d6360: 41 80 fb 3f cmp $0x3f,%r11b 24d6364: 0f 95 44 24 ee setne -0x12(%rsp) 24d6369: 80 fb 7b cmp $0x7b,%bl 24d636c: 41 0f 95 c3 setne %r11b 24d6370: 80 fb 7d cmp $0x7d,%bl 24d6373: 0f 95 44 24 ed setne -0x13(%rsp) 24d6378: 80 fb 20 cmp $0x20,%bl 24d637b: 0f 95 44 24 ec setne -0x14(%rsp) 24d6380: 31 c0 xor %eax,%eax 24d6382: 80 fb 09 cmp $0x9,%bl 24d6385: 74 54 je 24d63db <net::HttpUtil::IsToken(b ase::BasicStringPiece<std::string> const&)+0x10b> 24d6387: 44 22 4c 24 ef and -0x11(%rsp),%r9b 24d638c: 45 20 ca and %r9b,%r10b 24d638f: 45 20 f0 and %r14b,%r8b 24d6392: 45 20 d0 and %r10b,%r8b 24d6395: 44 20 f9 and %r15b,%cl 24d6398: 44 20 e1 and %r12b,%cl 24d639b: 44 20 e9 and %r13b,%cl 24d639e: 44 20 c1 and %r8b,%cl 24d63a1: 40 20 f2 and %sil,%dl 24d63a4: 40 20 fa and %dil,%dl 24d63a7: 40 20 ea and %bpl,%dl 24d63aa: 22 54 24 ee and -0x12(%rsp),%dl 24d63ae: 20 ca and %cl,%dl 24d63b0: 44 22 5c 24 ed and -0x13(%rsp),%r11b 24d63b5: 44 22 5c 24 ec and -0x14(%rsp),%r11b 24d63ba: 41 20 d3 and %dl,%r11b 24d63bd: 48 8b 4c 24 f8 mov -0x8(%rsp),%rcx 24d63c2: 48 8b 54 24 f0 mov -0x10(%rsp),%rdx 24d63c7: 74 12 je 24d63db <net::HttpUtil::IsToken(b ase::BasicStringPiece<std::string> const&)+0x10b> It initially seems like it might be doing something clever by stashing the results of the comparisons in registers, but then it starts to spill onto the stack and you realise it was stupid after all. Probably the test should be rewritten to be exhaustive, then IsTokenChar() can be changed to use a bitmask. In a perfect world, the compiler would do this for us and we wouldn't need to write the source code in a hard-to-understand manner just to get compact output. Other random code generation quality complaints: * The comparison with 0x9 at 24d6382 is redundant with the comparison against 0x1F at 24d6303. I also don't understand why that comparison gets its own branch. * The comparison with 0x20 at 24d6378 could be folded into the 0x1F comparison at 24d6303.
,
Aug 25 2016
mmenke: Rationally, no. Optimising things that aren't causing a problem is not a good use of time. Irrationally, yes. It's hard to unsee it once you've seen it.
,
Aug 25 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 28 2017
Now it uses a jump table: 0000000000219c70 <net::HttpUtil::IsTokenChar(char)>: 219c70: 40 80 ff 21 cmp $0x21,%dil 219c74: 7d 03 jge 219c79 <net::HttpUtil::IsTokenChar(char)+0x9> 219c76: 31 c0 xor %eax,%eax 219c78: c3 retq 219c79: 40 80 c7 de add $0xde,%dil 219c7d: 40 80 ff 5d cmp $0x5d,%dil 219c81: 77 17 ja 219c9a <net::HttpUtil::IsTokenChar(char)+0x2a> 219c83: 31 c0 xor %eax,%eax 219c85: 40 0f b6 cf movzbl %dil,%ecx 219c89: 48 8d 15 90 df 3b 00 lea 0x3bdf90(%rip),%rdx # 5d7c20 <net::HttpResponseHeaders::AddNonCacheableHeaders(std::__1::unordered_set<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >*) const::kPrefix+0x7a> 219c90: 48 63 0c 8a movslq (%rdx,%rcx,4),%rcx 219c94: 48 01 d1 add %rdx,%rcx 219c97: ff e1 jmpq *%rcx 219c99: c3 retq 219c9a: b0 01 mov $0x1,%al 219c9c: c3 retq Fortunately I've managed to stop caring.
,
Jul 6
,
Jul 6
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mmenke@chromium.org
, Aug 25 2016