blynn / nex

Lexer for Go
http://cs.stanford.edu/~blynn/nex/
GNU General Public License v3.0
416 stars 47 forks source link

Remove unnecessary busy waiting #55

Closed HerrSpace closed 3 years ago

HerrSpace commented 4 years ago

While fuzzing the mgmt[0] MCL lexer/parser I observed nex performing bad when used via go-fuzz. When calling the lexer myself from a normal main function I got acceptable execution speeds, but when utilizing it via go-fuzz the execution speeds were much worse. I debugged the issue with the go-fuzz developers[1] and @dvyukov figured out why this is happening.

go-fuzz sets GOMAXPROCS to 1 in each worker. When only using one thread to schedule nex code, the busy waiting loop will hog a lot of CPU time and it will take quite some time for the other go-routines to be able to unblock the busy-waiting go routine.

[0] https://github.com/purpleidea/mgmt [1] https://github.com/dvyukov/go-fuzz/issues/286

dvyukov commented 4 years ago

If only true is ever sent to ch_stop (?) then the code may be simplified even more as after the select we know that 1 of the things has happened, so we don't need break and outer loop.

purpleidea commented 4 years ago

@HerrSpace Thanks for the patch! I somehow got maintainer super-powers here, so I can merge this (AFAICT, it LGTM) but I'll give @blynn a chance to have a look first in case they're still around.

Do you have some details on your fuzzing setup so we can see what you did?

Thanks!

HerrSpace commented 4 years ago

@dvyukov You mean like that ^. I scrolled through the code and it indeed seems like we only ever send true to ch_stop.

HerrSpace commented 4 years ago

Do you have some details on your fuzzing setup so we can see what you did?

Of course, I'll write something up :)

purpleidea commented 4 years ago

I merged the first of the two commits! Thanks! I didn't merge the other since I didn't trace through all the code yet. TBH, since I'm not a brilliant computer scientist, I sort of avoid changing scary lexer code that I'm not familiar with which isn't breaking. :(