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

Issue #6273: Accessing POM via XPath is very slow #6274

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

reckart
Copy link
Contributor

@reckart reckart commented Sep 17, 2024

  • For the simple get() method, simply use DOM methods instead of going through XPath

I have tried to profile the issue again, but so far have not been able to encounter the situation where this get() method is called again. I even tried setting a breakpoint in the method while using Eclipse "in production" - but so far, the breakpoint did not hit again...

- For the simple get() method, simply use DOM methods instead of going through XPath

Signed-off-by: Richard Eckart de Castilho <[email protected]>
@chrisrueger
Copy link
Contributor

@reckart can you rebase master and push again? This will trigger the rebuild without the windows build (temporarily)

@chrisrueger
Copy link
Contributor

I triggered the Ubuntu builds manually. Successful.

@reckart could you elaborate on your comment

but so far have not been able to encounter the situation where this get() method is called again. I even tried setting a breakpoint in the method while using Eclipse "in production" - but so far, the breakpoint did not hit again...

@reckart
Copy link
Contributor Author

reckart commented Sep 19, 2024

Thing is that I do not really know under which conditions the code that I profiled to be a bottleneck is actually called (see screenshot in issue description). I wanted to check what kind of performance impact my change has - but I have not been able to trigger another situation where I would have been able to profile the same point.

@chrisrueger
Copy link
Contributor

Is there a test triggering this code?
Otherwise sounds like caching maybe so that stuff is only done initially? What if you delete/rename your local maven repo / eclipse workspace. Looks like m2e is involved. Not familiar with it

@reckart
Copy link
Contributor Author

reckart commented Sep 19, 2024

I initially had an NPE in the changes and that caused some tests to fail.

@chrisrueger
Copy link
Contributor

Sounds like at least no test fails.
If you are sure it is correct I'm fine with merging if you could have an eye on it and fix any potential issues in case.

@pkriens what do you think?

@pkriens
Copy link
Member

pkriens commented Sep 20, 2024

LGTM. What's up with the windows check being canceled?

@reckart
Copy link
Contributor Author

reckart commented Sep 20, 2024

So far I couldn't find an issue with the PR. Am I 100% sure that my code usually works - no. That said, I have been running Eclipse with a local build of the plugins installed for some days now and so far didn't hit an issue... 🤷

@chrisrueger
Copy link
Contributor

LGTM. What's up with the windows check being canceled?

This here was before you disabled the windows build in the other PR.

@pkriens pkriens merged commit 8955576 into bndtools:master Sep 20, 2024
8 of 9 checks passed
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.

3 participants