Bug 276326 - [target] Target resolution when editing targets
Summary: [target] Target resolution when editing targets
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.5   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.7 M2   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
: 287608 291085 (view as bug list)
Depends on:
Blocks: 295576 323612 324037
  Show dependency tree
 
Reported: 2009-05-14 11:13 EDT by Jeff McAffer CLA
Modified: 2010-09-13 11:25 EDT (History)
4 users (show)

See Also:


Attachments
Possible fix (16.96 KB, patch)
2010-08-19 17:06 EDT, Curtis Windatt CLA
no flags Details | Diff
Rollback Patch (17.37 KB, patch)
2010-08-25 15:02 EDT, Ankur Sharma CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff McAffer CLA 2009-05-14 11:13:48 EDT
It seems that every time a Target is edited in the preference pages there is a long (multi minute) delay while "Resolving Target Contents".  It appears that several repos are being downloaded (you can see contents.jar being downloaded) and the progress seems to say that various things are being "installed".  That part goes by quickly but looks like these are platform-specific fragments or some such.  Note that this seems to happen similarly when opening the target editor.

- p2 should be already caching and updating the repos as needed so redownloading every time the target it edited should not be needed.

- The profile has all the goodies in it for the user to edit so we should not need the remote repo at all unless the user wants to add more stuff from that particular site. 

In general users will expect the workflows and behaviours here to be similar to that of the Install Software experience.  There you can open the current profile and poke around pretty much without delay.  Repos are updated as needed and loaded in the background.
Comment 1 Darin Wright CLA 2009-05-14 11:27:28 EDT
Basically, the target definition does not know if all of its contents are local/in synch with the definition. So the contents are resolved each time the target is opened for editing in the editor/wizard. If any resolution was cancelled, or any properties of the target have changed (environment, etc.), the previous resolution could be invalid.
Comment 2 Jeff McAffer CLA 2009-05-14 11:30:18 EDT
related... 

Having added the elcipse updates repo to the target
   http://download.eclipse.org/eclipse/updates/3.5-I-builds
opening the target editor or doing any provisioning spends quite bit of time saying

Resolving Target Contents...: Fetching compositeArtifacts.jar (483B of 483B at 0B/s) from http://download.eclipse.org/eclipse/updates/3.5-I-builds/compositeArtifacts.jar

That file loads instantaneously from a web browser on the same machine.  It could just be unfortunate progress reporting but there does not appear to be much CPU or network activity...
Comment 3 Jeff McAffer CLA 2009-05-14 11:41:46 EDT
(In reply to comment #1)
> Basically, the target definition does not know if all of its contents are
> local/in synch with the definition. So the contents are resolved each time the
> target is opened for editing in the editor/wizard. If any resolution was
> cancelled, or any properties of the target have changed (environment, etc.),
> the previous resolution could be invalid.

I'm not sure what you mean here about resolution.  the profile is self-contained and already resolved nothing can/should change in the target without the user having "edited" it. you can show the user the content etc etc.  

To show the unselected elements of the location the repo content is needed. I'm pretty sure that p2 has support for updating repos only when needed.  As a result, there should be no need to re-download the contents.  

Comment 4 Darin Wright CLA 2009-05-14 11:47:56 EDT
The target definition is shared. One person adds a site location to it, and releases to CVS, the other person catches up with the new target definition. They both have local profiles, but there is no way to know that the definition is in synch with the profile.
Comment 5 Darin Wright CLA 2009-05-14 11:49:50 EDT
(In reply to comment #3)
> the profile is
> self-contained and already resolved nothing can/should change in the target
> without the user having "edited" it.

So this is the important point - we don't currently store metadata about when the target was edited, and when the profile was known to be good.
Comment 6 Jeff McAffer CLA 2009-05-14 11:55:45 EDT
If you knew that there was a change (resource delta) on the .target file then you would know that the profile is out of sync and needs to be updated.  This would be the common case if the targets are shared (they would be resources in the workspace).  A resource change listener that invalidates the profile when the associated .target is changed might do the trick here.

More generally setting a profile property that is the timestamp of the .target file would address this issue and shave huge amounts off the time to edit a target.
Comment 7 Curtis Windatt CLA 2010-08-19 17:06:51 EDT
Created attachment 177051 [details]
Possible fix

This patch allows the IUBundleContainers to get information out of the profile rather than doing a full resolve (planning/slicing).  This can drastically reduce the time it takes to resolve as there is no planning and no pinging of remote repositories.  If the profile does not exist, does not contain the correct ius, or a backing file in the bundle pool is missing, we silently fall back to a normal resolve.  This fix has the added benefit that we can successfully work offline until a normal resolve is required.

This patch has been applied to HEAD.  I still plan on writing some tests and maybe make some more tweaks.  It would be great for some people to try out the next I build and see if they observe any odd behaviour.
Comment 8 Curtis Windatt CLA 2010-08-19 17:08:12 EDT
One thing that we might be able to do better:  When a p2 location is removed, we delete the profile, which forces a full resolve.  If we avoiding deleting the profile, it would have stale data, but it wouldn't have to resolve.  We explicitly check for this in TargetDefinition.getProfile().  I don't remember why it was added, I don't suppose you remember Darin?
Comment 9 Darin Wright CLA 2010-08-20 10:40:30 EDT
(In reply to comment #8)
> One thing that we might be able to do better:  When a p2 location is removed,
> we delete the profile, which forces a full resolve.  If we avoiding deleting
> the profile, it would have stale data, but it wouldn't have to resolve.  We
> explicitly check for this in TargetDefinition.getProfile().  I don't remember
> why it was added, I don't suppose you remember Darin?

I think the logic here was "where we get stuff from might change" if a target references multiple repositories, has multiple root IUs, and a root IU is removed. However, this theoretically should not matter, as an artifact is the same no matter where it originates from.
Comment 10 Curtis Windatt CLA 2010-08-20 16:21:20 EDT
*** Bug 291085 has been marked as a duplicate of this bug. ***
Comment 11 Curtis Windatt CLA 2010-08-20 16:21:48 EDT
*** Bug 287608 has been marked as a duplicate of this bug. ***
Comment 12 Ankur Sharma CLA 2010-08-25 15:02:14 EDT
Created attachment 177460 [details]
Rollback Patch

The fix cause test failures hence rolling it back. See Bug 323612
Comment 13 Curtis Windatt CLA 2010-08-30 17:41:00 EDT
The test failures were due to some unexpected behaviour with the PermissiveSlicer.  If you take an empty profile and slice it, one would expect to always get back an empty array.  However, if you provide an existing IU to slice the requirements for, you get an array containing the IU you passed in.

I have put the code back into head and fixed this case by always finding the root IUs from the profile by looking up id/version rather than trusting that the cached IUs.

I opened bug 324037 to improve the tests and look for other cases where we might have stale information in the profile.