Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Enable prefetching childnodes #113

Merged
merged 9 commits into from

3 participants

@chregu
Owner

Jackrabbit has the possibility to fetch childnodes of a node in one
call. This could be used to improve performance, if you have a lot of
child nodes and want to fetch all (or most) of them anyway. You save at
least the roundtrip latency for all those requests (if you don't use
Session->getNodes, which is more complicated to code)

This is a patch, which enables that feature.

I'm open to change things, notably not sure if

$session->getTransport()->setFetchDepth($depth)

is the right thing to do here (but it somehow fits to the transport class)

See also:

jackalope/jackalope-jackrabbit#16

@dbu
Owner

i don't like the getTransport() part too much, its rather deep in implementation specifics. what about adding an option jackalope.fetch_depth and a hasSessionOption/getSessionOption/setSessionOption to the Session class that allows to overwrite changeable options (for now just fetch_depth but maybe we come up with more)?

the session would forward this option to the transport, i agree it belongs there.

this would be easier to integrate later if other implementations start to have that behaviour too. (create some ConfigurableSessionInterface and check for that rather than Jackalope\Session before calling the method)

what do you think?

@lsmith77
Owner

i don't have a good overview of how this would work in practice. but i would imagine that i might not want this for all nodes and i might want a different depth for different nodes. so especially when thinking about the ODM, i might want to be able to specify the depth on a per model basis.

@dbu
Owner

then the odm would call the setSessionOption before and after loading a node if it knows the model beforehand. the difficulty with the odm is that it does not know what it is loading until it has loaded it - unless you explicitly tell it what to expect.

i think in practice, it might be enough to just set a global depth for 95% of the cases and then do some special optimization for edge cases. with the odm we could say you just talk directly to the session.

@chregu
Owner

yeah, the idea is to do
$session->setSessionOption(..., 2)
$session->getNode("/foo/bar");
$session->setSessionOption(..., 0)

if you know you need more than depth 0. It's not meant to be as a global option (you still can do that of course) but on a per-request base.

@dbu
Owner

please make this example

if ($session instanceof Jackalope\Session && $session->hasSessionOption(...)) {
    $depth = $session->getSessionOption(...)
    $session->setSessionOption(..., 2)
    $session->getNode("/foo/bar");
    // or, for lukas example: $dm->find(null, '/foo/bar');
    $session->setSessionOption(..., $depth)
}

to avoid breaking with other implementations. but do we have agreement to do it like this?

@chregu
Owner

It actually would be

if ($session instanceof Jackalope\Session && $session->hasSessionOption(...)) {
    $depth = $session->getSessionOption(...)
    $session->setSessionOption(..., 2)
}

$session->getNode("/foo/bar");

if ($session instanceof Jackalope\Session && $session->hasSessionOption(...)) {
    // or, for lukas example: $dm->find(null, '/foo/bar');
    $session->setSessionOption(..., $depth)
}

which to me is too complicated. with at least the fetch_depth session option you could set that always, the code for the user doesn't change, if it's not supported. It just makes more calls to the backend but the code is the same

$depth = $session->getSessionOption(...)
$session->setSessionOption(..., 2)
 $session->getNode("/foo/bar");
$session->setSessionOption(..., $depth)

should be enough IMHO

@chregu
Owner

about breaking with other implementations, I see what you mean, if they use just PHPCR and not Jackalope (I often forget that)... mmmh. Adding this to PHPCR/Session would be the solution then to be able to takte getSessionOption method for granted.

mmmh mmh mmh, I'd prefer to be able to write simpler, but still portable code (but of course I'm free to use my code above, if I'm sure I never use non-jackalope related PHPCR-implementations)

@dbu
Owner

interfacing api's is powerful but it complicates things when you want to write really portable code. i am very reluctant to add things to PHPCR\SessionInterface "just like that" - there must be a reason JCR did not do it...

i am ok to say you can set any option even if it is unknown and just ignored it, which would at least get rid of the hasSessionOption call. shall we add the options like this and say unknown options just return null and are ignored when being set? then you still need the instanceof check for portable code but it gets a little bit simpler.

@chregu
Owner

Added Session->setSessionOption(), didn't change the transport stuff, it is now used by Session->setSessionOption

Ok so? will add code comments then :)

src/Jackalope/Session.php
@@ -691,4 +691,18 @@ public static function getSessionFromRegistry($key)
return self::$sessionRegistry[$key];
}
}
+
+ public function setSessionOption($key, $value) {
@lsmith77 Owner

opening curly should be on the next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Jackalope/Session.php
@@ -691,4 +691,18 @@ public static function getSessionFromRegistry($key)
return self::$sessionRegistry[$key];
}
}
+
+ public function setSessionOption($key, $value) {
+ if ($key == 'jackalope.fetch_depth') {
+ $this->getTransport()->setFetchDepth($value);
+ }
+
+ }
+
+ public function getSessionOption($key) {
@lsmith77 Owner

opening curly should be on the next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Owner

i agree we need to be careful about unilaterally adding methods to the PHPCR interfaces, if only because we might risk creating unnecessary differences with JCR if they eventually also add this feature.

as for ignoring options or not. in case of this option it would be safe for implementations to ignore it since its just a performance optimization. but there might be others which are more critical. so imho we should throw an exception if its not supported. however we can prevent any performance overhead at least in any setup that uses a DIC where we can check this during configuration of the DIC.

@chregu
Owner

@lsmith77 but with fetch_depth we shouldn't throw an exception, right? Wouldn't make a lot of sense here. I suggest we throw exceptions, when the behaviour changes

Apart from that, you're fine with the PR as it is (apart from the missing docblocks)

And I fixed the curlies..

@lsmith77
Owner

my point is we should expect PHPCR implementations to throw an exception if an unsupported option is set.

@chregu
Owner

But then if I want to write portable code, I'd had to do the following:

try {
  if ($session instanceof Jackalope\Session && $session->hasSessionOption(...)) {
    $depth = $session->getSessionOption(...)
    $session->setSessionOption(..., 2)
  }
} catch (NotSupportedException $e) {
}

$session->getNode("/foo/bar");

try {
  if ($session instanceof Jackalope\Session && $session->hasSessionOption(...)) {
      // or, for lukas example: $dm->find(null, '/foo/bar');
      $session->setSessionOption(..., $depth)
  }
} catch (NotSupportedException $e) {
}

a little very verbose for my taste. (which we can get rid of a little bit, if we make "hasSessionOption" part of PHPCR::Session, which this PR doesn't do. It's a Jackalope specific thing right now)

@lsmith77
Owner

yes you would have to .. except that if you configure the DIC you could determine if this option is supported or not for the configured service and then set an option in your code if to use this option. aka when we configure PHPCR ODM, we check if the implementation supports this option and if yes, we enable setting it in PHPCR ODM.

@dbu
Owner

when the implementation does not know the option it can not decide if it was a behaviour-changing one or not. so i'd say we require the implementation to throw an exception on unknown options, but it can ignore options it knows that are "just" about performance or other non behaviour-changing things. (as will be the case for jackalope-doctrine-dbal somehow. if we merge like this, it will set the fetch depth as this is in core jackalope and the doctrine transport will just ignore the setting)

could you please add a throw InvalidArgumentException "unknown option $key" at the end of the set and get methods? and please add a doc comment on the Session class too, explaining the behaviour and listing the (for now 1) option.

shouldn't we make the option name a class constant of Session?

@dbu
Owner

my proposition would avoid the try-catch in chregus example. we should document the throws for unsupported options like

 @ throws InvalidArgumentException if the option is unknown
 @ throws RepositoryException if this option is not supported and is a behaviour relevant option
@chregu
Owner

Added Docblocks and Exceptions.

About the Constants: I'm not a big fan of constants, if they just replace a string which doesn't change anyway in the future (or would change with the same probability as the constant due to a bad name). It just adds overhead without making anything easier (except maybe code completion in IDEs)

@dbu dbu merged commit 3fa3f26 into jackalope:master
@dbu
Owner

well, the good thing of a constant is that the php interpreter will tell you if you did a typo, whereas the string can be misspelled. and since working with phpstorm i came to really appreaciate code completion :-)

would you mind if i add a constant? in your own code you can still use the string directly - we will only complain if you want to use it inside any official code ;-)

@chregu
Owner

The overhead is added in the declaration of the const, not much in actually using it. But I won't object adding it.

@dbu
Owner

did the cleanup http://git.io/NM4mcQ
and added a note about it to the readme of jackalope-jackrabbit too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
24 src/Jackalope/ObjectManager.php
@@ -174,6 +174,8 @@ protected function resolveBackendPath($path)
* @param string $absPath The absolute path of the node to fetch.
* @param string $class The class of node to get. TODO: Is it sane to fetch
* data separately for Version and normal Node?
+ * @param object $object A (prefetched) object (de-serialized json) from the backend
+ * only to be used if we get child nodes in one backend call
*
* @return NodeInterface
*
@@ -184,7 +186,7 @@ protected function resolveBackendPath($path)
*
* @see Session::getNode()
*/
- public function getNodeByPath($absPath, $class = 'Node')
+ public function getNodeByPath($absPath, $class = 'Node', $object = null)
{
$this->verifyAbsolutePath($absPath);
$absPath = $this->normalizePath($absPath);
@@ -194,12 +196,28 @@ public function getNodeByPath($absPath, $class = 'Node')
return $this->objectsByPath[$class][$absPath];
}
- $fetchPath = $this->getFetchPath($absPath, $class); // will throw error if path is deleted
+ if (!$object) {
+ $fetchPath = $this->getFetchPath($absPath, $class); // will throw error if path is deleted
+ $object = $this->transport->getNode($fetchPath);
+ }
+
+ foreach ($object as $name => $properties) {
+ if (is_object($properties)) {
+ $objVars = get_object_vars($properties);
+ $countObjVars = count($objVars);
+ //load childnodes if there's more than one objectvar
+ // or just one and this isn't jcr:uuid, then we assume it was
+ // fetched from the backend completely
+ if ($countObjVars > 1 || ($countObjVars == 1 && !isset($objVars['jcr:uuid']))) {
+ $this->getNodeByPath($absPath . '/' . $name, $class, $properties);
+ }
+ }
+ }
$node = $this->factory->get(
$class,
array(
- $this->transport->getNode($fetchPath),
+ $object,
$absPath,
$this->session,
$this
View
45 src/Jackalope/Session.php
@@ -14,7 +14,9 @@
use PHPCR\PathNotFoundException;
use PHPCR\ItemNotFoundException;
use PHPCR\ItemExistsException;
+use PHPCR\RepositoryException;
use PHPCR\UnsupportedRepositoryOperationException;
+use InvalidArgumentException;
use PHPCR\Security\AccessControlException;
@@ -691,4 +693,47 @@ public static function getSessionFromRegistry($key)
return self::$sessionRegistry[$key];
}
}
+
+ /**
+ * Sets a session specific option.
+ * Currently only 'jackalope.fetch_depth' is supported. This option sets the depth with which
+ * nodes should be gotten from the backend.
+ *
+ * @param string $key the key to be set
+ * @param mixed $value the value to be set
+ *
+ * @throws InvalidArgumentException if the option is unknown
+ * @throws RepositoryException if this option is not supported and is a behaviour relevant option
+ *
+ * @see Jackalope\Transport\BaseTransport::setFetchDepth($value);
+ */
+
+ public function setSessionOption($key, $value)
+ {
+ if ($key == 'jackalope.fetch_depth') {
+ $this->getTransport()->setFetchDepth($value);
+ } else {
+ throw new InvalidArgumentException("Unknown option: $key");
+ }
+
+ }
+
+ /**
+ * Gets a session specific option.
+ *
+ * @param string $key the key to be gotten
+ *
+ * @throws InvalidArgumentException if the option is unknown
+ *
+ * @see setSessionOption($key, $value);
+ */
+
+ public function getSessionOption($key)
+ {
+ if ($key == 'jackalope.fetch_depth') {
+ return $this->getTransport()->getFetchDepth();
+ }
+
+ throw new InvalidArgumentException("Unknown option: $key");
+ }
}
View
24 src/Jackalope/Transport/BaseTransport.php
@@ -34,6 +34,16 @@
$/xi";
/**
+ * The current fetchDepth
+ *
+ * @var int
+ *
+ * @see TransportInterface::setFetchDepth($depth)
+ */
+
+ protected $fetchDepth = 0;
+
+ /**
* Helper method to check whether the path conforms to the specification
* and is supported by this implementation
*
@@ -99,5 +109,19 @@ public function assertValidName($name)
return true;
}
+ /**
+ * {@inheritDoc}
+ */
+ public function setFetchDepth($depth) {
+ $this->fetchDepth = $depth;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function getFetchDepth() {
+ return $this->fetchDepth;
+ }
+
// TODO: #46 add method to generate capabilities from implemented interfaces
}
View
25 src/Jackalope/Transport/TransportInterface.php
@@ -290,4 +290,29 @@ function setNodeTypeManager($nodeTypeManager);
*/
function getNodeTypes($nodeTypes = array());
+ /**
+ * Sets the depth with which a transport should fetch childnodes
+ * If depth = 0 it only fetches the requested node
+ * If depth = 1 it also fetches its children
+ * If depth = 2 it also fetches its children and grandchildren
+ * and so on
+ *
+ * Be aware the the actual Session->getNode call does not return all
+ * the children. This setting only tells the transport to preemptively
+ * fetch all the children from the backend.
+ *
+ * @param int $depth The depth with which the nodes should be fetched.
+ */
+
+ function setFetchDepth($depth);
+
+ /**
+ * Returns the current fetchDepth
+ *
+ * @return int with the current fetchDepth
+ *
+ * @see TransportInterface::setFetchDepth($depth)
+ */
+ function getFetchDepth();
+
}
Something went wrong with that request. Please try again.