Optimising my code away

#1

Anyone want to spot the problem with this optimisation?  I'm calling DirectoryContentsList.isStillLoading().  First the unoptimised code:

   jassert(directoryList != nullptr);

    while (directoryList->isStillLoading()) {}

5A171DF4  mov         ecx,dword ptr [this]  

5A171DF7  add         ecx,0F4h  

5A171DFD  call        juce::ScopedPointer<juce::XmlElement>::operator* (5A0B4630h)  

5A171E02  mov         ecx,eax  

5A171E04  call        juce::DirectoryContentsList::isStillLoading (5A2834B0h)  

5A171E09  movzx       eax,al  

5A171E0C  test        eax,eax  

5A171E0E  je          PresetBrowser::refresh+82h (5A171E12h)  

5A171E10  jmp         PresetBrowser::refresh+64h (5A171DF4h)

And now the optimised:

5A281DE0  mov         eax,dword ptr [eax+64h]  

   jassert(directoryList != nullptr);

    while (directoryList->isStillLoading()) {}

5A281DE3  test        eax,eax  

5A281DE5  jne         PresetBrowser::refresh+53h (5A281DE3h)

If I'm reading this right, it is the digital equivalent of guarding a bank by taking a picture, going to the pub and staring at the picture.   Am I reading it right?  

  • The function isStillLoading() does this: {return  fileFindHandle != nullptr;}
  • fileFindHandle is updated by another thread. 
  • The compiler misses this entirely and copies fileFIndHandle directly into eax and then keeps checking eax to see if it's turned into a nullptr yet. 
  • It doesn't.

I can disable optimisation for this function, but this is so broken ... is there something I'm doing wrong with the complier? 

J.

#2

I'm not looking at the asm but


while (directoryList->isStillLoading()) {}

this looks not good to me, cause its blocking the message thread. Better use a timer instead.

#3

It's only pausing a fraction of a second in the GUI after it's saved a new preset - hasn't caused any problems to date.  But you are right I could deal with this in a different way by refreshing the menu and let it read the disk in the background.  I'll have a think about that, it'll work find where I'm using a tree view I think, but I'm not sure when i'm refreshing the popupmenu with the presets that there's an easy way of updating a popupmenu which is already popped?

#4

I am interested in the solution to the class of problem though.  I can fix this particular instance in a number of ways.

However, I'm worried my lack of knowledge about this problem  will cause subtle bugs in other places. 

#5

Never, never run a tight loop like that!

And if this is the message thread, you should never run a loop of any kind at all - always use a timer or other callback to wait for things.

The compiler is treating it as a single operation, and deciding that it's ok to cache the result of the method call - probably if you stuck a sleep(1) in the middle, it'd be unable to make that optimisation. But really, you shouldn't be writing code like that in the first place.

#6

Okay, fixed it as suggested by both of you.

I still find the problem rather interesting though.  It feels like there's a gap that something like 'volatile' ought to fill to ensure that you get the desired behaviour out of a multi-threaded programme.  Surely there are situations when blocking until another thread completes is a reasonable course of action?

 

#7

Surely there are situations when blocking until another thread completes is a reasonable course of action?

Nope. Maybe if you're implementing an OS kernel or something, but not at the app level.

The 'volatile' keyword is generally avoided in C++ these days - the way to do it in C++11 is via the new memory model and atomic<>. But yes, you're right - to be strictly correct, I should have made the pointer atomic to force the compiler to treat it as being volatile.

#8

Thank you. 

Good to know how to handle it in case it does come up.  Still, the while loop is now history, consigned to the dustbin (though archived in git forever...)