Query #80
Query #80
Conversation
…bit ASCII characters. Queries may or may not not find strings that contain non-ASCII characters.
Conflicts: test/performance/timer.cpp test/performance/timer.hpp
…t testing and cleanup
ghost
assigned 
| @@ -212,6 +207,14 @@ The aggregate_local() function contains a tight loop that tests the condition of | |||
| return av; | |||
| } | |||
|
|
|||
| inline size_t LocalEnd(size_t global_end) | |||
There was a problem hiding this comment.
'inline' is implied here. No need to specify it. This is true whenever the function definition is placed inside a class scope.
There was a problem hiding this comment.
We have agreed to use underscore separated function names, so please at least do that for new functions such as this one. Ie. this one should have been called 'local_end()'.
There was a problem hiding this comment.
I think you're thinking about semantics of "inline" and not about code generation. Defining a method in class scope does not hint the compiler to inline it during code generation. If the method is large enough (enough line numbers), the compiler will issue call/ret/etc. instructions to a common version.
I've changed it to TIGHTDB_FORCEINLINE instead...
There was a problem hiding this comment.
Your change is cool with me, but I maintain that the 'inline' keyword is redundant when specified on a method definition (as oppopsed to a method declaration) inside a class scope. Please let me know if you disagree.
There was a problem hiding this comment.
At some method-size treshold, the compiler will no longer inline the method but create a call instruction instead. In VS, for the method below, you get, for no 'inline' keyword:
s = m_cse.m_array_ptr->find_first(m_key_ndx, s - m_cse.m_leaf_start, m_cse.LocalEnd(end
000000013F1F226A mov rdi,qword ptr [rbx+188h]
000000013F1F2271 lea rcx,[rbx+168h]
000000013F1F2278 mov rdx,r12
000000013F1F227B mov qword ptr [rsp+40h],r15
000000013F1F2280 mov qword ptr [rsp+48h],0FFFFFFFFFFFFFFFFh
000000013F1F2289 call tightdb::SequentialGetter<__int64>::LocalEnd (13F1F42A0h)
compared to this for forceinline:
s = m_cse.m_array_ptr->find_first(m_key_ndx, s - m_cse.m_leaf_start, m_cse.LocalEnd(end));
000000013FA444DA cmp rsi,1
000000013FA444DE jne tightdb::StringNodetightdb::Equal::find_first_local+149h (13FA444E9h)
000000013FA444E0 lea r9d,[rsi+1]
000000013FA444E4 jmp tightdb::StringNodetightdb::Equal::find_first_local+22Eh (13FA445CEh)
000000013FA444E9 cmp rsi,2
000000013FA444ED jne tightdb::StringNodetightdb::Equal::find_first_local+157h (13FA444F7h)
000000013FA444EF mov r9,r14
000000013FA444F2 jmp tightdb::StringNodetightdb::Equal::find_first_local+22Eh (13FA445CEh)
size_t LocalEnd(size_t global_end)
{
if(global_end == 1)
return 2;
if(global_end == 2)
return 0;
if(global_end == 6)
return 4;
if(global_end == 8)
return 2;
if(global_end == 5)
return 8;
if(global_end == 2)
return 6;
if(global_end == 1)
return 4;
if(global_end == 8)
return 2;
if(global_end == 9)
return 4;
if(global_end == 11)
return 2;
if(global_end == 222)
return 0;
if(global_end == 644)
return 4;
if(global_end == 855)
return 2;
if(global_end == 543)
return 8;
if(global_end == 232)
return 6;
if(global_end == 12)
return 4;
if(global_end == 89)
return 2;
if(global_end == 91)
return 4;
if (global_end > m_leaf_end)
return m_leaf_end - m_leaf_start;
else
return global_end - m_leaf_start;
}
There was a problem hiding this comment.
Yes, what you are illustrating nicely is that __forceinline in VS makes the expected difference, but that is beside my point. I was and am still talking about the standardized 'inline' keyword. And, I still maintain that it is redundant when specified on a method definition that resides inside a class scope. Only when you move the definition out of the class, does the 'inline' keyword make a difference. Let me know if you still disagree.
There was a problem hiding this comment.
Camel case is fixed (github was hiding some comments...)
I'm in doubt about 'inline' because the C++ standard only talks about observable semantics, not code generation. So explicit 'inline' could still have an effect. Testing needed :)
| const AdaptiveStringColumn* asc = static_cast<const AdaptiveStringColumn*>(m_condition_column); | ||
| if(s >= m_end_s) { | ||
| // we exceeded current leaf's range | ||
| delete(m_leaf); |
There was a problem hiding this comment.
Sir, are you choosing this form deliberately? I'll just remind you that 'delete' is an operator, and that the intended and most common way of writing it, is 'delete m_leaf'. This form would improve readability for me. If you change it, remember the others in this file.
There was a problem hiding this comment.
I have no preference, but I don't really want to bother changing it. .feel free to change it if you prefer, though
|
+1 |
Optimized speed for queries on enum, long and short strings