Skip to content

Merge allgress query analysis branch to tonsky.#12

Closed
allgress wants to merge 1 commit intotonsky:masterfrom
allgress:master
Closed

Merge allgress query analysis branch to tonsky.#12
allgress wants to merge 1 commit intotonsky:masterfrom
allgress:master

Conversation

@allgress
Copy link
Copy Markdown

This branch adds query analysis and the ability to listen only for transactions that would affect a particular query. The query analysis remembers which parts of the index are used in the query, and transacted datoms are analyzed to see if they update any of these. May be a useful performance optimization, rather than notifying all listeners of all transactions and leaving it to the listener to query against tx-data, decide if any changes are relevant, etc.

Not sure if this is the best approach, particularly since the logic is essentially duplicated between the query and analysis functions. Also some annoying whitespace changes I didn't catch before committing, sorry.

@narma
Copy link
Copy Markdown

narma commented Jun 15, 2014

Small tip about whitespace changes, to view diff via github without whitespace changes just add w param with true value to URL, for example: https://github.com/tonsky/datascript/pull/12/files?w=true

@tonsky tonsky force-pushed the master branch 4 times, most recently from 0f63209 to 031e4f7 Compare August 27, 2014 11:54
@EwenG
Copy link
Copy Markdown

EwenG commented Aug 27, 2014

Thanks for your work on this allgress. I made a few updates there https://github.com/EwenG/datascript/tree/analyze-q. Updates include:

  • Merge with the 0.2 version of datascript
  • listen! takes the index keys as argument instead of a query. I think this make the listen! function more flexible.
  • The callbacks are stored in a map from indexkeys to callbacks. This allows the transact! logic to notify the listeners using a few map lookups instead of iterating all listeners.
  • I added a few other things like the defquery macro (syntactic sugar around analyze-q).

I don't know if this should be merged into datascript. I still think there is something wrong with modifying the listen! API. But I am also concerned with how datascript would scale when a lot of callbacks are registered, especially in the context of a GUI where a lot of events can be generated (think mouse dragging events). I would love to know tonsky opinion on this.

@tonsky
Copy link
Copy Markdown
Owner

tonsky commented Aug 27, 2014

This is excerpt from email discussion with @allgress :

I also think that part of proper query analysis (the one you’re doing by extracting indexes) is quite complex and hard to get exactly correct (it’ll always be more of an approximation). So may be there should be simpler API, like, subscribe for datoms of this entity (by eid) or this attribute (by attr).

I’m not sure that the whole idea of “reverse” query can be solved, even in datalog. But much simpler approach, like (listen! conn '[_ :user/name _]) or (listen! conn [room-id :room/messages '_]) might be doable — and, not ideal, but a huge win with a lot of use-cases. I’m going to look into it soon.

@EwenG
Copy link
Copy Markdown

EwenG commented Aug 27, 2014

Isn't it what allgress "index keys" already do? analyze-q is just syntactic sugar to extract those index keys ('[_ :user/name ] or '[room-id :room/messages ']) from a query.

This is indeed an approximation since it is "only" a static analysis.

@tonsky
Copy link
Copy Markdown
Owner

tonsky commented Aug 28, 2014

@EwenG sorry, haven’t looked at your code before, without analyse-q your API looks like what I’ve planned, because we don’t promise too much, only subscription to index we can efficiently support. I’ll look closer into your implementation and report back

@kahunamoore
Copy link
Copy Markdown

This sounds a lot like what the RETE algorithm does - it effectively builds a custom network/indexes that exactly fit the set of queries/rules known to the system and shares common sub expressions for efficiency. Trades memory for runtime.

@jeluard
Copy link
Copy Markdown

jeluard commented Apr 21, 2015

Are you still considering this feature? Looks extremely useful.

@tonsky
Copy link
Copy Markdown
Owner

tonsky commented Apr 21, 2015

@jeluard yes, but haven’t got time to implement it or do a review on PR. At some point, something like this, yes

@atroche
Copy link
Copy Markdown

atroche commented Sep 5, 2015

@allgress are you guys still using this for your own apps? how is it working for you?

@sparkofreason
Copy link
Copy Markdown

@atroche We're using Datascript, but not the allgress branch with query-based notifications. We've moved away from the single state atom approach, and have really embraced the immutable aspect of Datascript. Our web components might receive an instance of a Datascript DB via an attribute from a parent, or create it themselves through a service call. Child components often publish relevant changes through events, from which listeners can update their own instance as appropriate. Since we use freactive, sometimes we'll make a lens which represents a specific query or "view" on the DB.

We've found that this approach allows us to keep the Datascript instances fairly small (for our apps, at least), and haven't had any perf issues which would require query-specific notifications.

@kahunamoore
Copy link
Copy Markdown

No, we are using the Clara rules engine which gives you these features (and more) by design.

@metasoarous
Copy link
Copy Markdown

Hadn't seen this thread before... I'll just chime in to mention that this sounds a lot like parts of https://github.com/mpdairy/posh. In fact, there's an analyze-q there as well with output results that look a lot like these, so I'm wondering if @mpdairy actually looked at that in his implementation. To my knowledge he hasn't hooked any of that up for generalized listeners, but there's a single listener that goes through the active subscriptions, checks datoms against the generated patterns, and decides whether or not to update the queries based on this. So, if anyone reading this thread is interested in this line of thinking, you may want to check out Posh.

That having been said, I'd like to point out that there's some discussion on #132 in the direction of incremental materialized view maintenance which would go one step further in actually preventing re-computation of queries, instead using previous results and tx-diffs to compute the new query results far more efficiently.

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.

9 participants