Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import BND packages instead of require Bundle biz.aQute.bndlib in pde.ui #775

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

HannesWell
Copy link
Member

No description provided.

@github-actions
Copy link

Test Results

     273 files  ±0       273 suites  ±0   1h 5m 10s ⏱️ + 4m 18s
  3 340 tests ±0    3 309 ✔️ +1  30 💤 ±0  0 ±0  1 🔥  - 1 
10 317 runs  ±0  10 226 ✔️ +1  90 💤 ±0  0 ±0  1 🔥  - 1 

For more details on these errors, see this check.

Results for commit 71053c8. ± Comparison against base commit d8a3ad4.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, no clue why I used require-bundle here...

@vogella
Copy link
Contributor

vogella commented Sep 29, 2023

Will this make the resolver faster? IIRC resolving import package used to very slow and I still have to use -Dequinox.resolver.revision.batch.size=1 for a lot of client code, e.g., if I run unit tests to avoid long startup-times in Equinox. cc @tjwatson

@laeubi
Copy link
Contributor

laeubi commented Sep 29, 2023

Will this make the resolver faster?

It should at least make the resolver allowing better decisions.

IIRC resolving import package used to very slow and I still have to use -Dequinox.resolver.revision.batch.size=1 for a lot of client code,

I don't think this is directly connected to import package but to overall system setup and constrains. Still I'm always a bit confused why the resolver can be made that slow and a batchsize of 1 works without an issue. As mentioned on the other issue I think the resolver should simply always use batchsize=1 and only use a larger batch (probably reuse information from the previous step) if this does not resolve sucessfully.

Also I'm not sure if it is applicable to the resolver, for P2 I could speedup things considerably by first making a preliminary scan for dependencies that might match and then only perform a resolve operation on that reduced set as often the solution the is trivial because all req have a 1:1 match ...

@vogella
Copy link
Contributor

vogella commented Sep 29, 2023

Will this make the resolver faster?

It should at least make the resolver allowing better decisions.

IIRC resolving import package used to very slow and I still have to use -Dequinox.resolver.revision.batch.size=1 for a lot of client code,

I don't think this is directly connected to import package but to overall system setup and constrains. Still I'm always a bit confused why the resolver can be made that slow and a batchsize of 1 works without an issue. As mentioned on the other issue I think the resolver should simply always use batchsize=1 and only use a larger batch (probably reuse information from the previous step) if this does not resolve sucessfully.

Also I'm not sure if it is applicable to the resolver, for P2 I could speedup things considerably by first making a preliminary scan for dependencies that might match and then only perform a resolve operation on that reduced set as often the solution the is trivial because all req have a 1:1 match ...

@tjwatson WDYT? Can we make batchsize 1 the default? Seems like > 50 % of my clients need this setting to start their RCP stuff in a reasonable time.

@tjwatson
Copy link
Contributor

Will this make the resolver faster?

It should at least make the resolver allowing better decisions.

I predict this will make the resolution slower if you have multiple versions of biz.aQute.bndlib installed. It only adds more decision branches to be made to the tree while resolving. Not that I want to advocate for the use of Require-Bundle.

@tjwatson WDYT? Can we make batchsize 1 the default? Seems like > 50 % of my clients need this setting to start their RCP stuff in a reasonable time.

I think a better approach would be to have p2 do a full resolution any time it installs new stuff (mimicking what a -clean would do). I recall having a discussion like that in p2 at some point? Usually when things are slow to resolve after installing new stuff a -clean launch solves the problem because it removes previous resolution decisions allowing the resolver to make better choices from the start leading to finding the solution faster.

@tjwatson
Copy link
Contributor

I think a better approach would be to have p2 do a full resolution any time it installs new stuff

Seems I did that a long time ago (https://bugs.eclipse.org/bugs/show_bug.cgi?id=565066). But it didn't have the desired effect on the scenario where I installed a later version of biz.aQute.bndlib. I will need to investigate why. But I cannot do that until next week.

@HannesWell
Copy link
Member Author

I think a better approach would be to have p2 do a full resolution any time it installs new stuff

Seems I did that a long time ago (https://bugs.eclipse.org/bugs/show_bug.cgi?id=565066). But it didn't have the desired effect on the scenario where I installed a later version of biz.aQute.bndlib. I will need to investigate why. But I cannot do that until next week.

Sounds good from a high level perspective, but I currently have no insights in the details.

@HannesWell HannesWell merged commit 6b0f0ed into eclipse-pde:master Sep 29, 2023
11 of 14 checks passed
@HannesWell HannesWell deleted the importBND branch September 29, 2023 14:51
@laeubi
Copy link
Contributor

laeubi commented Sep 30, 2023

I think a better approach would be to have p2 do a full resolution any time it installs new stuff (mimicking what a -clean would do).

See for example this issue regarding clean in general:

beside this, I think it would be good to handle this in equinox itself, can't we have a flag that is set whenever new bundles are installed to clean the stored state after next (cold) restart?

Maybe one don't need to clear everything if Equinox knows the bundles affected, but I think its fair to have a refresh after a process restart in such case, I can't really think about a case where it would be useful to retain the old resolver state as it is actually only meant as a caching? Maybe one can have a "refreshcache" option with

  • always
  • never
  • oninstall (default)

or similar just for the case there are use-cases that require the old behavior.

@HannesWell
Copy link
Member Author

I think it would be good to handle this in equinox itself, can't we have a flag that is set whenever new bundles are installed to clean the stored state after next (cold) restart?

I assumed that this would happen anyways.^^

But sounds all reasonable to me. Should we open an Equinox/P2 issue for that to discuss it there?

@merks
Copy link
Contributor

merks commented Sep 30, 2023

Note that there is already work planned about restart behavior so this might fit into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants