search

salsa-rs / salsa

A generic framework for on-demand, incrementalized computation. Inspired by adapton, glimmer, and rustc's query system.
https://salsa-rs.netlify.app/
Apache License 2.0
2.09k stars 142 forks source link

chore: use tracing instead of log #516

Closed davidbarsky closed 1 month ago

davidbarsky commented 2 months ago

Besides my well-known biases, rust-analyzer uses tracing, and given how big of a difference Salsa 3.0 it, I figured it makes sense to switch over.

(We switched over a few months back in rust-analyzer's vendored fork of Salsa.)

netlify[bot] commented 2 months ago

Deploy Preview for salsa-rs canceled.

Name Link
Latest commit 86f06d8485edd3ad829a186eadb9fee91dbcf32c
Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/669ed7d88b1ae300082c5444
nikomatsakis commented 1 month ago

@davidbarsky I'm happy-ish to merge this PR (I'm a bit grumpy that we don't have a clearer standard in Rust land, but that's neither your fault nor tracing's, really) but it needs a rebase =)

davidbarsky commented 1 month ago

@davidbarsky I'm happy-ish to merge this PR (I'm a bit grumpy that we don't have a clearer standard in Rust land, but that's neither your fault nor tracing's, really) but it needs a rebase =)

Rebased! And yeah: I think they occupy their own, slightly distinct niches at this point (particularly from an FFI perspective), but I wouldn't mind resolving those gaps.

MichaReiser commented 1 month ago

I'm not sure if it is intentional but this log can blow up, depending on the value (in our case, it logs the entire AST :laughing:).

https://github.com/MichaReiser/salsa/blob/2f4f80fe23cfb968ffe58fdf99aa77eacf6dbf9a/src/function/maybe_changed_after.rs#L109

Should we exclude the value from the Memo's Debug impl?

davidbarsky commented 1 month ago

I'm not sure if it is intentional but this log can blow up, depending on the value (in our case, it logs the entire AST 😆).

https://github.com/MichaReiser/salsa/blob/2f4f80fe23cfb968ffe58fdf99aa77eacf6dbf9a/src/function/maybe_changed_after.rs#L109

Should we exclude the value from the Memo's Debug impl?

@MichaReiser Yes, it's probably a good idea to exclude it. sorry about that!