Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple logging methods #63

Merged
merged 4 commits into from
Oct 31, 2018
Merged

Conversation

nikomatsakis
Copy link
Member

This is an experimental branch. It introduces a method salsa_event into the Database trait that defaults to a no-op. This event is given a closure (event_fn) which, when called, will produce information about what is happening -- the purpose of the closure is to defer any real work (notably, cloning the descriptor) if the salsa_event is not enabled.

This could be used for various purposes. For example, tests can override salsa_event and log the events somewhere, using them to reconstruct what happened. I was imagining though that we might provide some higher-level consumers at some point.

This branch uses salsa_event in the parallel test to force one thread blocks for another, which we could not do before.

@nikomatsakis nikomatsakis mentioned this pull request Oct 23, 2018
@nikomatsakis
Copy link
Member Author

One thing I did not like about this interface: there is no way to inspect the kind of the event before you invoke event_fn. An earlier draft had multiple methods (one per callback kind) -- maybe that is better, and they should have default implements that invoke the "default" salsa_event callback. Alternatively, salsa::Event could have a lifetime associated with it, or use Cow values, but those options seem kind of annoying.

src/lib.rs Show resolved Hide resolved
src/runtime.rs Show resolved Hide resolved
@nikomatsakis
Copy link
Member Author

OK, so, I added Debug impls and also added per-event methods (salsa_event_foo) so that you can easily intercept just the event kinds that you like. I also refactored the Event structure to move some more common data into one place (e.g., the descriptor that appears in every event).

I'm feeling pretty "ok" about this change though mildly nervous about some of the details (e.g., just when do events fire etc).

I'd like to have a "gc collected" event but that's a bit of a pain to thread through (doable, just takes a bit more thought).

I'd prefer if the "event delivery" methods were in a separate trait, but lacking specialization there is no easy way to do that without all database implementations having to add an impl for it, which don't really want.

I guess overall I'm inclined to merge but this seems like the sort of thing we might want to revisit before 1.0 and reconsider.

@nikomatsakis
Copy link
Member Author

OK, I've removed the "per event kind" methods. That seemed like a lot more surface area than was warranted.

@kleimkuhler
Copy link
Contributor

I like these changes. I agree that the utility of a "gc collected" event would be nice. I have a few questions about where the complexity occurs specifically for that, but I'm sure it will become a little more clear as I use the events.

@nikomatsakis nikomatsakis merged commit 2d8dbce into salsa-rs:master Oct 31, 2018
nikomatsakis added a commit to nikomatsakis/salsa that referenced this pull request Nov 1, 2018
- major refactoring to the database APIs for safer parallel
  processing (salsa-rs#78, salsa-rs#82):
  - To set an input, you now write `db.query_mut(Query).set(...)`,
    and you must declare your database as `mut`.
  - To fork a thread, you now write `db.snapshot()`, which acquires
    a read-lock that is only released when the snapshot is dropped
    (note that this read-lock blocks `set` from occuring on the main
    thread).
  - Therefore, there can only be one mutable handle to the
    database; all other handles are snapshots. This eliminates a variety
    of complex and error-prone usage patterns.
- introduced the `salsa_event` callback that can be used for logging
  and introspection (salsa-rs#63)
@nikomatsakis nikomatsakis mentioned this pull request Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants