search

rune-rs / rune

An embeddable dynamic programming language for Rust.
https://rune-rs.github.io
Apache License 2.0
1.7k stars 85 forks source link

rune: Do not trace return value (causes panic on Result:Err) #748

Closed VorpalBlade closed 1 month ago

VorpalBlade commented 1 month ago

This is the fix for #747 on the main branch (removal of instrumenting the return value). I left the instrument itself in, though this could be removed instead if it isn't useful to keep that around.

udoprog commented 1 month ago

Thanks!

Wouldn't fixing the fmt::Debug impl for Value make this unnecessary?

VorpalBlade commented 1 month ago

Hm, fair point, I work primarily on 0.13.x, not main. So that variant is more well tested. However I don't think it would currently crash on main. But if you somehow manage to return a moved value it would. Not sure if that is possible.

VorpalBlade commented 1 month ago

Do you want to remove the tracing change now? It shouldn't crash without that any more, but I admit to not having tested that yet. Even if it doesn't crash, how does every return attempting to format the value affect the performance?

udoprog commented 1 month ago

Do you want to remove the tracing change now? It shouldn't crash without that any more, but I admit to not having tested that yet. Even if it doesn't crash, how does every return attempting to format the value affect the performance?

It should only affect performance if tracing is enabled. Otherwise it should be negligible. Tracing can also be statically compiled away with feature flags if we want to maximize performance.