Bug #79635

Coalescence calls to processDatamap_afterDatabaseOperations

Added by Thomas Hohn over 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
Category:
DataHandler aka TCEmain
Target version:
Start date:
2017-02-06
Due date:
% Done:

100%

TYPO3 Version:
8
PHP Version:
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

Due to a bug in the Datahandler - extra expensive processing of the hook
processDatamap_afterDatabaseOperations can occur for remapping calls.

Current situation in vanilla TYPO3 installation:
Create page with 1 tt_content element. Copy that page.
This triggers 1 call to processDatamap_afterDatabaseOperations for the page - but also 3 calls to
processDatamap_afterDatabaseOperations for the tt_content element.

It is possible to coalescent calls to processDatamap_afterDatabaseOperations
into one pr. table instead of one pr.
remapstack entry in processRemapStack

It would be better only to issue 1 call for the tt_content element with the "combined" processing array
since each subscriber would need to filter the other 2 calls if only the "signal" that a change
on the table has occurred.

The solution is to add coalescent the entries into one record.

processRemapStack.php View (7.3 KB) Thomas Hohn, 2017-03-09 13:15

processRemapStack.log View (14.7 KB) Thomas Hohn, 2017-03-09 13:15

Associated revisions

Revision 0dfac264 (diff)
Added by Thomas Hohn over 2 years ago

[BUGFIX] Coalesce hook calls in DataHandler::processRemapStack()

DataHandler's hook processDatamap_afterDatabaseOperations is processed
in two ways. In case modifications do not contain any new relation that
just has been created, the hook is executed directly. If that's not the
case, executing this hook is deferred and will happen after the remap
stack has been processed.

Calling the hook directly happens exactly once for each modified record,
where invocations in DataHandler::processRemapStack() might happen more
than once, depending on the amount of relation fields that contain new
references and have been remapped.

This change coalesces these invocations which results that the hooks
processDatamap_afterDatabaseOperations is exactly called once for each
modified record - which is the expected behavior.

Change-Id: Ib7e65ce170c8f9ba8f7577b79073b1ed9213a0b9
Resolves: #79635
Releases: master, 7.6
Reviewed-on: https://review.typo3.org/51552
Tested-by: TYPO3com <>
Reviewed-by: Markus Klein <>
Reviewed-by: Thomas Hohn <>
Tested-by: Thomas Hohn <>
Reviewed-by: Frank Naegler <>
Tested-by: Oliver Hader <>
Reviewed-by: Oliver Hader <>

Revision 4a7c5a9f (diff)
Added by Thomas Hohn over 2 years ago

[BUGFIX] Coalesce hook calls in DataHandler::processRemapStack()

DataHandler's hook processDatamap_afterDatabaseOperations is processed
in two ways. In case modifications do not contain any new relation that
just has been created, the hook is executed directly. If that's not the
case, executing this hook is deferred and will happen after the remap
stack has been processed.

Calling the hook directly happens exactly once for each modified record,
where invocations in DataHandler::processRemapStack() might happen more
than once, depending on the amount of relation fields that contain new
references and have been remapped.

This change coalesces these invocations which results that the hooks
processDatamap_afterDatabaseOperations is exactly called once for each
modified record - which is the expected behavior.

Change-Id: Ib7e65ce170c8f9ba8f7577b79073b1ed9213a0b9
Resolves: #79635
Releases: master, 7.6
Reviewed-on: https://review.typo3.org/51552
Reviewed-on: https://review.typo3.org/52257
Tested-by: TYPO3com <>
Reviewed-by: Oliver Hader <>
Tested-by: Oliver Hader <>

History

#1 Updated by Thomas Hohn over 2 years ago

  • Tracker changed from Feature to Bug
  • TYPO3 Version set to 8
  • Is Regression set to No

#2 Updated by Thomas Hohn over 2 years ago

  • Tracker changed from Bug to Feature

#3 Updated by Thomas Hohn over 2 years ago

  • Subject changed from Squash calls to processDatamap_afterDatabaseOperations to Squash calls to processDatamap_afterDatabaseOperations into one
  • Description updated (diff)

#4 Updated by Gerrit Code Review over 2 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#5 Updated by Thomas Hohn over 2 years ago

  • Subject changed from Squash calls to processDatamap_afterDatabaseOperations into one to Coalesce calls to processDatamap_afterDatabaseOperations into one
  • Description updated (diff)

#6 Updated by Thomas Hohn over 2 years ago

  • Subject changed from Coalesce calls to processDatamap_afterDatabaseOperations into one to Coalescence calls to processDatamap_afterDatabaseOperations into one
  • Description updated (diff)

#7 Updated by Gerrit Code Review over 2 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#8 Updated by Gerrit Code Review over 2 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#9 Updated by Gerrit Code Review over 2 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#10 Updated by Thomas Hohn over 2 years ago

  • Tracker changed from Feature to Task
  • Subject changed from Coalescence calls to processDatamap_afterDatabaseOperations into one to Wrap processDatamap_afterDatabaseOperations into new method
  • Description updated (diff)
  • TYPO3 Version set to 8

#11 Updated by Gerrit Code Review over 2 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#12 Updated by Gerrit Code Review over 2 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#13 Updated by Gerrit Code Review over 2 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#14 Updated by Gerrit Code Review over 2 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#15 Updated by Thomas Hohn over 2 years ago

  • Tracker changed from Task to Feature
  • Subject changed from Wrap processDatamap_afterDatabaseOperations into new method to Coalescence calls to processDatamap_afterDatabaseOperations
  • Description updated (diff)

#16 Updated by Benni Mack over 2 years ago

  • Target version changed from 8.6 to 8 LTS

#17 Updated by Riccardo De Contardi over 2 years ago

  • Target version changed from 8 LTS to 9.0

#18 Updated by Thomas Hohn over 2 years ago

  • Tracker changed from Feature to Bug
  • Description updated (diff)
  • Target version changed from 9.0 to 8 LTS
  • TYPO3 Version set to 8
  • Is Regression set to No

#19 Updated by Thomas Hohn over 2 years ago

  • Description updated (diff)

#20 Updated by Thomas Hohn over 2 years ago

  • Description updated (diff)

#21 Updated by Gerrit Code Review over 2 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#22 Updated by Gerrit Code Review over 2 years ago

Patch set 10 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#23 Updated by Gerrit Code Review over 2 years ago

Patch set 11 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#25 Updated by Thomas Hohn over 2 years ago

Took a plain TYPO3 8.6.1 installation. And modified the public function processRemapStack() as attached which yields the log files that indicates that
nearly the same data is sent with the hook. The proposed patch just ensures that the processing array contains the aggregation of the $hookArgs['fieldArray'].
As far as I can see the same data is posted multiple times via the hook - which gives a performance degradation and issues unnecessary processing
of the hook

#26 Updated by Oliver Hader over 2 years ago

@Kasper: Interesting slides which show that around 40% of processing time is wasted in EXT:solr or XCLASSes...

#27 Updated by Kasper Ligaard over 2 years ago

@Oliver: The take away from the slide-show should be that all the problems found have been solved and merged. The exceptions is this issue and the three other TYPO3 DataHandler issue presented on slide 11. (The slide-show was presented as part of the TYPO3 8 LTS Code Sprint we hosted at our Aarhus offices two weeks ago; Helmut Hummel and Nicole Cordes saw the presentation).

The Solr issue has been fixed in Solr. The xclass is also Solr, since Solr har a prolem; we are in close contact with Timo Hund and dkd and contribute patches to solve the Solr problems (See our 100+ accepted pull requests here, https://github.com/TYPO3-Solr/ext-solr/pulls?utf8=%E2%9C%93&q=is%3Apr%20author%3Athomashohn%20 - it cover the last few months only!).

So you can disregard both the Solr and XClassing problems are solved now.

PS: Also note that the slide-show does not delve into the host of other DataHandler and RefIndexer issues we have solved and contributed over the last 3-4 months; you can browse our recent contributions here: https://review.typo3.org/#/q/owner:thomas%2540hohn.dk+branch:master+OR+owner:%22Christer+%253Cchrisv10%2540gmail.com%253E%22+branch:master+OR+owner:%22Claus+Due+%253Cclaus%2540phpmind.net%253E%22+branch:master - we would greatly appreciate if you also find time to review some of the still-not-merged issues in this list :-)

#28 Updated by Oliver Hader over 2 years ago

@Kasper, thanks for the information and I appreciate your dedication and contribution for TYPO3. Especially when DataHandler as low-level boss-class is involved we need to be very careful since only tiny adjustments could lead to inconsistent data and relations. I'm aware of the fact, that the DataHandler is not optimal in terms of software architecture, that's why I was working on event sourcing in the 2nd and 3rd quarter of 2016 (https://www.slideshare.net/ohader/typo3-event-sourcing, e.g. slide 22 shows nested calls for copying a record). But introducing that is nothing that could be done on the fly and not for CMS 8 anymore.

#29 Updated by Oliver Hader over 2 years ago

For the current issue, we need to clear what currently happens, what should happen and what the patch currently does.

The problems you ran into are caused by the remapStack in DataHandler that triggers the hook (if registered) for each relation field that contains at least one new reference (a record that has been copied during the process). I consider this as a bug since the calling the hook should happen for a complete record that has been modified once and not for each field.

Combining the hook calls for all records of the same table would be a new feature and lead to a new hook - however, the current implementation of the patch behaves differently depending on being called inside or outside of the remapStack method.

From my point of view, this issue should concentrate on combining the calls to the hook inside remapStack without introducing a new hook or any new behavior. I can support with writing functional tests to assert the the concerning hook of this issue is called correctly with the expected data.

#30 Updated by Gerrit Code Review over 2 years ago

Patch set 12 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#31 Updated by Gerrit Code Review over 2 years ago

Patch set 13 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#32 Updated by Kasper Ligaard over 2 years ago

@Oliver: I believe Thomas Hohn with patch set 13 accomodates the concerns you raised, i.e. only fixing the performance bug without introducing new or changed behaviour. To me the code looks correct, but I think that only you and Thomas has the required deep knowledge of DataHandler to be able to truly verify.

Also, we (and TYPO3) would greatly appreciate if you find the time to do some functional tests for this part of TYPO3; we would be prepared to compensate you for your time, if done before TYPO3 8 LTS.

#33 Updated by Gerrit Code Review over 2 years ago

Patch set 14 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#34 Updated by Gerrit Code Review over 2 years ago

Patch set 15 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#35 Updated by Thomas Hohn over 2 years ago

  • Description updated (diff)

#36 Updated by Gerrit Code Review over 2 years ago

Patch set 16 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/51552

#37 Updated by Gerrit Code Review over 2 years ago

Patch set 1 for branch TYPO3_7-6 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/52257

#38 Updated by Thomas Hohn over 2 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#39 Updated by Riccardo De Contardi about 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF