only parse info.xml once #30188

butonic · 2018-01-19T07:08:40Z

I was profiling the preview stuff and noticed that we are parsing all info.xml files twice per request.

Top left: current state
Top rigth: cache in local variable per request for file as well.
Bottom left: This PR, cold cache
Bottom right: cached info.xml

x6 speed increase

Related: #30190

codecov · 2018-01-19T07:36:11Z

Codecov Report

Merging #30188 into master will increase coverage by 0.02%.
The diff coverage is 91.13%.

@@             Coverage Diff              @@
##             master   #30188      +/-   ##
============================================
+ Coverage     64.81%   64.83%   +0.02%     
+ Complexity    18378    18319      -59     
============================================
  Files          1199     1198       -1     
  Lines         69583    69558      -25     
  Branches       1282     1282              
============================================
- Hits          45099    45098       -1     
+ Misses        24111    24087      -24     
  Partials        373      373

Flag	Coverage Δ	Complexity Δ
#javascript	`53.08% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.19% <91.13%> (+0.02%)`	`18319 <3> (-59)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Repair.php	`26.19% <ø> (+0.3%)`	`21 <0> (ø)`	⬇️
lib/private/App/CodeChecker/InfoChecker.php	`94.44% <ø> (ø)`	`18 <0> (ø)`	⬇️
lib/private/Installer.php	`48.18% <20%> (-0.95%)`	`83 <0> (-4)`
lib/private/legacy/app.php	`63.8% <84.61%> (-0.75%)`	`163 <0> (-17)`
lib/private/App/AppManager.php	`90.76% <98.36%> (+4.91%)`	`52 <3> (-31)`	⬇️
apps/files_trashbin/lib/Expiration.php	`96.55% <0%> (-1.73%)`	`29% <0%> (ø)`
...eratedfilesharing/lib/Controller/OcmController.php	`66.06% <0%> (-0.21%)`	`30% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe1e27...2f5212d. Read the comment docs.

mrow4a · 2018-01-23T08:00:23Z

:O interesting, 👍 for idea, let's see when you finish the PR

cdamken · 2018-01-23T17:45:29Z

Testing it and runs fine and faster than b4

butonic · 2018-01-23T22:14:34Z

yay, finally found the problem, now really ready to review

mrow4a · 2018-01-24T06:18:37Z

I remember @DeepDiver1975 telling me never to add lines in legacy and move the code to new code base. So you think this extra effort is possible here ?

mrow4a

I added few comments to better understand this part of code

mrow4a · 2018-01-24T06:25:42Z

lib/private/legacy/app.php

-		if ($path) {
+	public static function getAppInfo($appId, $isPath = false) {
+		if(empty($appId)) {
+			return null; // TODO explode?


#30190 - invalid xml produces a version of 0 ... causing migrations to not be applied
#30204 - but it needs a more in depth effort to make it explode properly / do the right thing if it explodes

So TODO needs to stay there?

as a reminder, for anyone looking at the code, yes

lib/private/legacy/app.php

tests/data/app/expected-info.json

butonic · 2018-01-24T08:59:09Z

Let me see if I can move this to AppManager

patrickjahns · 2018-11-27T13:17:37Z

Some more thoughts on this:

What happens when we upgrade an app via occ ?
What happenes when we upgrade/install an app via occ market:install ?

butonic · 2018-11-27T13:33:50Z

@patrickjahns the cache is invalidated if an info.xml etag changes. see https://github.com/owncloud/core/pull/30188/files#diff-8fa91b147fc76360cb3cfca41a1db57dR406

PVince81 · 2019-02-08T15:43:40Z

@butonic please recheck for outstanding comments/tasks so someone else can take over.

butonic · 2019-02-12T12:20:23Z

@PVince81 I rebased. All comments should be addressed. Please take over. Fingers crossed about the testsuite.

PVince81 · 2019-02-12T13:38:51Z

@VicDeo please takeover as you're already familiar with the matter

lib/private/App/CodeChecker/InfoChecker.php

VicDeo · 2019-02-12T18:47:19Z

lib/private/App/AppManager.php

-		$appInfo = \OC_App::getAppInfo($appId);
-		if ($appInfo === null) {
-			return null;
+		if (empty($appId)) {


empty is not good for strings

well ... ok ... i did not want to check for null, false and emptystring ... and an app id with 0 or an empty array is wrong as well .. so i think this kind of makes sense here ... maybe do if (!is_string($appId) || $appId === '')?

one of rare cases when strict comparison sucks :)

ok, I went with (!is_string($appId) || $appId === '')

VicDeo · 2019-02-12T18:49:25Z

Great job in general, just two minor things

butonic · 2019-02-12T21:28:26Z

@VicDeo both resolved

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

VicDeo

changed is_string to \is_string to pass CI. The rest is good.

VicDeo · 2019-02-13T09:37:39Z

@PVince81 backport?

VicDeo · 2019-02-14T11:08:03Z

Stable10: #34482

butonic added 2 - Developing performance labels Jan 19, 2018

butonic self-assigned this Jan 19, 2018

butonic force-pushed the only-parse-info.xml-once branch from b064dad to 88f431a Compare January 19, 2018 15:55

butonic requested review from DeepDiver1975, tomneedham and mrow4a January 19, 2018 15:56

butonic force-pushed the only-parse-info.xml-once branch 4 times, most recently from fa4b82e to a0c381d Compare January 19, 2018 21:22

butonic requested a review from VicDeo January 19, 2018 21:34

butonic mentioned this pull request Jan 19, 2018

[WIP] Explode on info.xml problems #30204

Closed

butonic changed the title ~~[WIP] only parse info.xml once~~ only parse info.xml once Jan 19, 2018

butonic added 3 - To Review and removed 2 - Developing labels Jan 19, 2018

butonic force-pushed the only-parse-info.xml-once branch 3 times, most recently from 8c7a171 to c6af155 Compare January 22, 2018 09:36

mrow4a reviewed Jan 24, 2018

View reviewed changes

ownclouders added the status/STALE label Feb 1, 2018

ownclouders added status/STALE and removed status/STALE labels Feb 9, 2018

butonic force-pushed the only-parse-info.xml-once branch from fcd8571 to 4dbddb5 Compare February 13, 2018 10:07

butonic force-pushed the only-parse-info.xml-once branch from c2442ed to b4d37ae Compare June 14, 2018 13:32

ownclouders added the status/STALE label Jun 22, 2018

ownclouders removed the status/STALE label Aug 30, 2018

PVince81 assigned butonic Feb 8, 2019

butonic force-pushed the only-parse-info.xml-once branch from b4d37ae to 5e54ea0 Compare February 12, 2019 12:07

PVince81 assigned VicDeo and unassigned butonic Feb 12, 2019

VicDeo reviewed Feb 12, 2019

View reviewed changes

lib/private/App/CodeChecker/InfoChecker.php Outdated Show resolved Hide resolved

VicDeo reviewed Feb 12, 2019

View reviewed changes

butonic force-pushed the only-parse-info.xml-once branch 2 times, most recently from 024caa0 to 678f54d Compare February 12, 2019 21:18

VicDeo force-pushed the only-parse-info.xml-once branch from 678f54d to d3fe459 Compare February 13, 2019 08:42

only parse info.xml once, store it in local cache

2f5212d

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>

VicDeo force-pushed the only-parse-info.xml-once branch from d3fe459 to 2f5212d Compare February 13, 2019 08:55

VicDeo approved these changes Feb 13, 2019

View reviewed changes

VicDeo merged commit d7a09c4 into master Feb 13, 2019

delete-merged-branch bot deleted the only-parse-info.xml-once branch February 13, 2019 09:37

VicDeo added the backport-request label Feb 14, 2019

VicDeo mentioned this pull request Feb 14, 2019

[Stable10] only parse info.xml once #34482

Merged

VicDeo removed the backport-request label Feb 14, 2019

lock bot locked as resolved and limited conversation to collaborators Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only parse info.xml once #30188

only parse info.xml once #30188

butonic commented Jan 19, 2018 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading

mrow4a commented Jan 23, 2018 •

edited

Loading

cdamken commented Jan 23, 2018

butonic commented Jan 23, 2018

mrow4a commented Jan 24, 2018

mrow4a left a comment

mrow4a Jan 24, 2018

butonic Jan 24, 2018

mrow4a Jan 24, 2018

butonic Jan 24, 2018

butonic commented Jan 24, 2018

patrickjahns commented Nov 27, 2018

butonic commented Nov 27, 2018

PVince81 commented Feb 8, 2019

butonic commented Feb 12, 2019

PVince81 commented Feb 12, 2019

VicDeo Feb 12, 2019 •

edited

Loading

butonic Feb 12, 2019 •

edited

Loading

VicDeo Feb 12, 2019

butonic Feb 12, 2019

VicDeo commented Feb 12, 2019

butonic commented Feb 12, 2019

VicDeo left a comment

VicDeo commented Feb 13, 2019

VicDeo commented Feb 14, 2019

only parse info.xml once #30188

only parse info.xml once #30188

Conversation

butonic commented Jan 19, 2018 • edited Loading

codecov bot commented Jan 19, 2018 • edited Loading

Codecov Report

mrow4a commented Jan 23, 2018 • edited Loading

cdamken commented Jan 23, 2018

butonic commented Jan 23, 2018

mrow4a commented Jan 24, 2018

mrow4a left a comment

Choose a reason for hiding this comment

mrow4a Jan 24, 2018

Choose a reason for hiding this comment

butonic Jan 24, 2018

Choose a reason for hiding this comment

mrow4a Jan 24, 2018

Choose a reason for hiding this comment

butonic Jan 24, 2018

Choose a reason for hiding this comment

butonic commented Jan 24, 2018

patrickjahns commented Nov 27, 2018

butonic commented Nov 27, 2018

PVince81 commented Feb 8, 2019

butonic commented Feb 12, 2019

PVince81 commented Feb 12, 2019

VicDeo Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

butonic Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

VicDeo Feb 12, 2019

Choose a reason for hiding this comment

butonic Feb 12, 2019

Choose a reason for hiding this comment

VicDeo commented Feb 12, 2019

butonic commented Feb 12, 2019

VicDeo left a comment

Choose a reason for hiding this comment

VicDeo commented Feb 13, 2019

VicDeo commented Feb 14, 2019

butonic commented Jan 19, 2018 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading

mrow4a commented Jan 23, 2018 •

edited

Loading

VicDeo Feb 12, 2019 •

edited

Loading

butonic Feb 12, 2019 •

edited

Loading