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

Response identity map #301

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jgaskins
Copy link
Contributor

This pull introduces/changes:

  • Response-level identity map (currently only in Responses::HTTP)

Given: (:Person)-[:WROTE]->(:Article) (with 1:N relationships)

When I run the following query:

Person
  .as(:person)
  .where(id: author_id)
  .articles(:article)
  .pluck(:person, :article)

I get a different instance of Person for every Article in the result set. It's reasonable to assume we can return the same instance for all references to the same node in the same result. This PR is a proof of concept for that in the hopes that I didn't overlook any context behind yielding new instances each time. :-) Additional benefits include saving a bit of RAM and probably some CPU time since we can skip the conversion from hash to ActiveNode/ActiveRel on subsequent occurrences of the same node in the result.

I didn't test whether nodes/rels in paths would use the cached instances, but if not, maybe that's a relatively simple change? I mainly wanted to get thoughts on this before I waded in too deep. :-)

Identity maps can be gross because cache invalidation is hard, but the scope of caching here is pretty small. It only sits at the response level, so it'll be thrown away with the response and subsequent queries will return fresh instances. Per-query identity seems like a reasonable compromise between performance and flipping tables over frustration caused by cache-invalidation bugs.

Pings:
@cheerfulstoic
@subvertallchris

When response rows reference the same node or relationship, the response
currently returns new object instances for each one. Since they're all
part of the same response, it's reasonable to assume they can be the
same object, so this identity-map prototype makes that happen, saving a
bit of RAM and possibly a bit of CPU time for the conversion from hash
to ActiveNode/ActiveRel.
@coveralls
Copy link

coveralls commented Sep 24, 2017

Coverage Status

Coverage increased (+0.04%) to 73.718% when pulling 0123743 on jgaskins:response-identity-map into 44f0754 on neo4jrb:master.

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.

2 participants