logger icon indicating copy to clipboard operation
logger copied to clipboard

Performance issue in `deparse_to_one_line`

Open deeenes opened this issue 3 years ago • 2 comments

Hello,

I was after a severe performance issue in one of my applications, and I ended up at the above mentioned function in logger. Within there, the gsub call can take eventually minutes (!). It happens if, due to nested calls or large vectors or data structures in the arguments, deparse returns a string of 10-100k characters long. Applying a regex containing many *'s is just computationally heavy.

I see the reasons why this has been introduced in response to #20. I think a number of solutions are possible:

  • Don't remove the multiple spaces, or do it only if the string is short
  • Truncate the deparsed string to a reasonable length (few hundreds of characters), I don't believe including calls longer than that is very useful in a log
  • The layout generator functions could check if the layout requires fn or call and pass this information to get_logger_meta_variables (all fields are always generated, but only these two, especially call, is expensive to process)

I think a combination of the second two would be a nice solution, i.e. if call is not required, we do nothing, otherwise we do the formatting on a truncated string. Let me know if I should make a PR.

Best,

Denes

deeenes avatar May 23 '22 22:05 deeenes

Thanks for the report.

I'd rather not do (1) or (2), but something like (3) should do the trick I think: get_logger_meta_variables should get the explicit list of fields to look up and return, as this way it's clear what is to be looked up and not wasting resources.

For this end, we need to add a fields param to get_logger_meta_variables and then update the calling layout functions, e.g. the layout_glue_generator should first extract the field names between the curly braces and pass to get_logger_meta_variables, and this is even easier with the JSON layouts.

Would you be open to creating a PR? Otherwise, I'll try to get there in a few weeks.

daroczig avatar May 24 '22 09:05 daroczig

I had to revert the above PR (see d2d7d58c12fb2fcd3014bda1395b1baaa703d27c for more details), so reopening the ticket to track future work.

daroczig avatar Mar 03 '24 21:03 daroczig