-
Notifications
You must be signed in to change notification settings - Fork 61
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
[POC] Lazy load property #284
base: 2.x
Are you sure you want to change the base?
Conversation
73272a1
to
a3bd1f3
Compare
@@ -886,7 +890,7 @@ public function hasProperty($relPath) | |||
$this->checkState(); | |||
|
|||
if (false === strpos($relPath, '/')) { | |||
return isset($this->properties[$relPath]); | |||
return isset($this->properties[$relPath]) || isset($this->propertyData[$relPath]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you even need to check $this->properties ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. .this was from an earlier stage .. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually ->setProperty
sets properties
directly.
interesting! i could not resist and had a look at it. i think what you currently miss is that when new properties are added, you also need to add their names to the $propertiesData, or you will "forget" them in some cases. |
a3bd1f3
to
1436c1f
Compare
@@ -737,13 +744,13 @@ public function getProperties($nameFilter = null) | |||
{ | |||
$this->checkState(); | |||
|
|||
//OPTIMIZE: lazy iterator? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you do below does not create a lazy iterator. but probably we don't need one.
dont you need to add information to the raw data as well when creating a new property: https://github.com/jackalope/jackalope/blob/lazy_load_property/src/Jackalope/Node.php#L1546-L1561 and what about deleting a property that has not been loaded yet? |
I didn't think so, as we always try to load first from
maybe, the tests are still failing on the implementation(s) and I am not sure why. |
Note to future self: |
I have reduced the errors to 7 with 5 failures, it requires probably only a few fixes before it "works". But its a bit of a mess, and I am sure I will introduce regressions somehow with the current work. But maybe it would be interesting for benchmarking and seeing if its worth finishing off. |
return array(); | ||
} | ||
$res = array(); | ||
$ntm = $this->session->getWorkspace()->getNodeTypeManager(); | ||
foreach ($this->properties['jcr:mixinTypes']->getValue() as $type) { | ||
foreach ($this->getPropertyValue('jcr:mixinTypes') as $type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tricky part is knowing when we can call these "API" methods, as they will always check the state and break thiings if the operation is "internal" and doesn't care about the external state.
Hmm, and a quick benchmark doesn't show any improvement currently, so I guess something is going wrong. |
This PR experiments with lazy-loading properties -- do not review :)