ENH Switch Parent to getParent method to make SiteTree faster

[sunnysideup]

This change will make Silverstripe much faster (i hope), because it uses SiteTree::getParent() instead of the magic / custom method Parent()

[GuySartorelli]

It'd be interesting to see some profiling to see if this does actually make a measurable I provement in performance

[sunnysideup]

@GuySartorelli - in my application it seems to make make the page request much faster. However, even if it was the same, I would recommend implementing the change because Parent is a generically / custom / magically created method based on a has_one, while getParent is a specific SiteTree method that can be optimised for SiteTree objects.

[sunnysideup]

@GuySartorelli - I would love you to try it on a larger site. I just tried it on a random site we run. I opened the home page with
?flush=all so that the menu, etc... is not cached. Without this fix, the time was 1.60s to load, with this fix it was around 1.45s (running both around 10 times to get an average value). I'd be interested to see what you get.

[wilr]

I can replicate the improvement on a stock standard install.

^ Parent()->Link took:
  0.001104

^ getParent()->Link took
  0.000040

^ Parent()->Link after cached (2nd call) took:
  0.000057

^ getParent()->Link after cached (2nd call) took
  0.000023

SiteTree defines a getParent(), so does Hierarchy (we should probably remove it from SiteTree).

The performance improvement probably shouldn't be shocking - Both those methods bypass the magic layer of Silverstripe (CustomMethods::__call()) For comparison, $page->getComponent('Parent') is what happens under the hood on the __call() dataobject bypassing most of the config / extension work and that is slower

[sunnysideup]

@GuySartorelli - what might be useful is if the automated tests also included some sort of timing feedback. I guess that is super rough, but it may give some indication on how fast things run with the proposed changes.

[sunnysideup]

@wilr - thank you for looking at this. I think the best gains are made in PHP code where we write:

$parent = $this->Parent();
if($parent && $parent->exists()) {
    //....
}

we should encourage people to write:

$parent = $this->getParent();
if($parent) {
    //....
}

This is not really part of this proposed change, but it does implement that idea withi SiteTree itself to make the code simpler and more consistent.

[sunnysideup]

Just running this again. I will make sure, of course, all tests pass. Just relying on this to pick up any issues.

[emteknetnz]

I like this PR, there's some changes to make

[emteknetnz]

Suggested change

	return SiteTree::get_by_id(self::class, $this->ParentID);
	return SiteTree::get()->byID($this->ParentID);

Slightly nicer syntax

[sunnysideup]

There is a difference between byID and get_by_id - from the code:

// The object returned is cached, unlike DataObject::get()->byID() {@link DataList::byID()}

[sunnysideup]

I have made all the changes as requested.

[michalkleiner]

Looks ok from my perspective, nice improvement.

[GuySartorelli]

Looks good in general. @sunnysideup can you please take a look at the tests? There's a lot of cases now where null is returned when the tests were previously getting pages.

[dhensby]

I'm not a huge fan of this kind of change. In reality the magic methods should be improved so they perform in a more optimal way throughout the framework (ie: make use of in-memory caches like this implementation does) rather than fixing this on a case-by-case basis.

This is one of the biggest regressions going from v3 to v4 of SS; the lack of caching of relationships/ORM queries.

[GuySartorelli]

Retargetted to 5 and rebased to see what the CI tests think of this PR in its current state

[GuySartorelli]

I've added some protection against null values to resolve most of the test failures.

While I do agree with @dhensby that the ORM could afford to be more efficient, with perhaps some built in caching in places it doesn't currently have it, I don't think that's something we can sensibly do in a minor release.
This change is though - and since the getParent() method already exists it seems sensible to start using it.

The failed test

The remaining failed test is failing because we've cached the query for the parent when the parent for that ID doesn't actually exist (because it hasn't yet been restored) - and then after restoring it, when we query for the parent, we're told it doesn't exist (even though now it does).

I think this PR is opening ourselves to a bunch of small regressions like that one, and while it does represent a performance gain, I think there's too much chance of regression for my tastes here.

I'm going to close this PR for now - but if someone thinks of a way we can mitigate this class of regression, I'd be happy to discuss and potentially reopen the PR.