Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 351944

Summary: [performance][repository] SimpleArtifactRepository checks local artifacts.xml/jar all the time if on a readonly filesystem
Product: [Eclipse Project] Equinox Reporter: Helmut J. Haigermoser <helmut.haigermoser>
Component: p2Assignee: Meng Xin Zhu <kane.zhu>
Status: CLOSED FIXED QA Contact:
Severity: critical    
Priority: P3 CC: irbull, mober.at+eclipse, pascal, tjwatson
Version: unspecified   
Target Milestone: Juno M5   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 366781    
Bug Blocks:    
Attachments:
Description Flags
a patch with unit test none

Description Helmut J. Haigermoser CLA 2011-07-13 06:52:01 EDT 
Build Identifier: I20110613-1736

A huge performance problem hit my installer, formerly based on 3.6, now migrated to 3.7: A call to calculate needed disk space (using SimpleArti.getArtifactDescriptors(key)) went from 2 seconds to 140 seconds.

Debugging the problem showed p2 to try and load the arti repo all the time, since I was installing from a network filesystem, readonly and rather slow, compared to a local disk. Each check for a modification takes 40ms there, which quickly sums up to the huge numbers I was seeing...

Looking at the SimlpeArtiRepo implementation I recommend not rechecking the local files with every single call to "getArtifactDescriptors(key)", more generally thinking a read-only arti-repo should probably be trusted to stay the same in-between getArtifactDescriptors(key) , what usecase would be caught by constantly checking the read-only location, other instances messing with the repo? Not sure that would bode well anyway since all information gathered till we find a repo to be modified would be out-of-date, and client codes have no way of detecting that..

Let me know what you think, I think we should at least support the property "eclipse.p2.internal.simple.artifact.repository.locking" throughout the code so that customers like my installer can disable it after finding issues...


Reproducible: Always

Steps to Reproduce:
1. have a slow, read-only filesystem (nfs for me, on a windows client)
2. in code have a few hundred keys from metadata, query arti repo with getArtifactDescriptors to get the descriptors (and the size as property)
3. calculate time it takes for each call to return (for me 40-60ms)
Comment 1 Pascal Rapicault CLA 2011-07-18 11:45:43 EDT 
One way to improve the situation could be to check if the drive is readonly when the repo is loaded and cache this until the repo is unloaded from memory.
Comment 2 Helmut J. Haigermoser CLA 2011-08-11 12:28:19 EDT 
(In reply to comment #1)
> One way to improve the situation could be to check if the drive is readonly
> when the repo is loaded and cache this until the repo is unloaded from memory.

All,
I've run into the second, terrible performance problem with this change, this time I'm not sure how to best work around it, since it's p2 code the calls the artifact repos(DownloadManager::getRequestsForRepository):

<code>
		for (IArtifactRequest request : requestsToProcess) {
			if (repository.contains(request.getArtifactKey()))
				applicable.add(request);
		}

</code>

What this does to me is loading my arti repos from a slow disk , times the number of artifacts to install :(

This quickly snowballs into a collect phase taking as much as 5 minutes until it even starts to fetch artifacts, which then is done in under a minute.

I guess I could derive from the DownloadManager and override the behavior to adapt to the performance bug, but ideally I would like to use a 3.7.x fix, like Pascal is proposing: do not re-load from read-only drives, at least if a certain property is set or not set...

Helmut
Comment 3 Pascal Rapicault CLA 2011-08-11 18:18:57 EDT 
Can you confirm that the situation you are interested in is one where the drive is really read only?
Comment 4 Helmut J. Haigermoser CLA 2011-08-12 04:00:39 EDT 
(In reply to comment #3)
> Can you confirm that the situation you are interested in is one where the drive
> is really read only?

Confirm, the location is a samba share, mounted read-only. It's considered local (no http)...
Helmut
Comment 5 Martin Oberhuber CLA 2011-11-16 15:05:40 EST 
Ping, is this something that could be considered for Indigo SR 2 ?
Comment 6 Meng Xin Zhu CLA 2011-11-21 03:04:56 EST 
Created attachment 207286 [details]
a patch with unit test

checking whether the repository can't be locked when initializing the location. If the repository can't be locked at all, it never reloads the repository when doing query and getting artifact keys.
Comment 7 Meng Xin Zhu CLA 2011-11-27 21:33:14 EST 
Ian and Pascal, do you have objections for my proposed fix? 

Thanks.
Comment 9 Ian Bull CLA 2011-12-05 18:17:16 EST 
I've reverted this commit for now.  Turns out that one of the test cases uses a 1.6 API.  Since our compiler compliance is set to 1.5, this caused the build to fail.

This should be pretty easy to fix, but because builds have been breaking all day -- and we really need a good build out, I simply reverted it for now.
Comment 10 Meng Xin Zhu CLA 2011-12-05 21:50:31 EST 
(In reply to comment #9)
> I've reverted this commit for now.  Turns out that one of the test cases uses a
> 1.6 API.  Since our compiler compliance is set to 1.5, this caused the build to
> fail.
> 
> This should be pretty easy to fix, but because builds have been breaking all day
> -- and we really need a good build out, I simply reverted it for now.
Sorry for breaking the build. I'll rework on the fix.
How do I subscribe the result of nightly build and unit test of equinox/p2?
Comment 11 Ian Bull CLA 2011-12-05 22:56:02 EST 
(In reply to comment #10)
> Sorry for breaking the build. I'll rework on the fix.
> How do I subscribe the result of nightly build and unit test of equinox/p2?

No worries.

Subscribe to the Eclipse releng mailing list [1]. Since this is milestone week, there might not have been a nightly build. This only appeared to be a problem in the test case, and likely pretty easy to fix. However, there were other problems with the build (not our fault), so I thought it was best to revert and not get in the way ;).

[1] https://dev.eclipse.org/mailman/listinfo/platform-releng-dev
Comment 12 Meng Xin Zhu CLA 2011-12-07 02:27:51 EST 
Released the fix again without using Java 6 API.

http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=f57a1651a3ed5baa7393d575b7f8726b31707d28
Comment 13 Ian Bull CLA 2011-12-07 11:42:48 EST 
(In reply to comment #12)
> Released the fix again without using Java 6 API.
> 
> http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=f57a1651a3ed5baa7393d575b7f8726b31707d28

Due to the timing (wednesday of Milestone week), I'm going to merge this into our integration branch next week.
Comment 14 Meng Xin Zhu CLA 2011-12-15 02:17:41 EST 
It's partially fixed. On windows it still has same problem like before. Bug 366781 reports the issue to check whether a location is read-only or not on Windows.
Comment 15 Meng Xin Zhu CLA 2011-12-18 22:45:04 EST 
Bug 366781 is fixed, close it.
Comment 16 Martin Oberhuber CLA 2012-01-24 04:39:44 EST 
The target milestone is empty, for what milestone has this been fixed ?

I'd assume Juno M4 but I don't know for sure ... and, would a backport into 3.7.2 be possible ?

Thanks,
Martin
Comment 17 Meng Xin Zhu CLA 2012-01-29 01:19:32 EST 
Indigo SR2 is in phase of RC2.

The backport need be approved by PMC.
Comment 18 Ian Bull CLA 2012-01-29 11:22:11 EST 
It seems very late to include this in the 3.7.2 SDK.  I'm not even sure we have the available number of committers to review the patch at this point.  Meng, I would suggest attending the Equinox/p2 call tomorrow and we can talk. 

http://wiki.eclipse.org/Equinox_Meeting_Minutes
Comment 19 Ian Bull CLA 2012-01-29 11:22:37 EST 
Adding Tom.
Comment 20 Thomas Watson CLA 2012-01-30 08:40:53 EST 
At this point I am not in favor of releasing this for 3.7.2.  We can discuss at today's call.