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

[performance] BoundSet: move properTypesByInferenceVariable before loop #3597

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Jan 22, 2025

as it does not depend on the loop variables

improves #3590 drastically

@jukzi
Copy link
Contributor Author

jukzi commented Jan 22, 2025

reduces reproducer #3594 (comment) from 17.7s to 5s

after:
image

@stephan-herrmann
Copy link
Contributor

I'll take a look, though not right now ...

Copy link
Contributor

@stephan-herrmann stephan-herrmann left a comment

Choose a reason for hiding this comment

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

The change is technically sound.

In addition to the cleanup request, I'd like to see one more measurement:

You reported that the extreme case from #3594 (comment) benefits from the change, good.

What about "normal" usage? The computation properTypesByInferenceVariable() will now be performed always, even if its result is never used. Does this method appear in any measurements of, e.g., GenericsRegressionTest_1_8?

as it does not depend on the loop variables

improves eclipse-jdt#3590
drastically
@jukzi jukzi force-pushed the properTypesByInferenceVariable branch from e62b428 to d73bf0a Compare February 10, 2025 08:11
@jukzi
Copy link
Contributor Author

jukzi commented Feb 10, 2025

What about "normal" usage? The computation properTypesByInferenceVariable() will now be performed always, even if its result is never used. Does this method appear in any measurements of, e.g., GenericsRegressionTest_1_8?

I executed GenericsRegressionTest_1_8 with and without this PR and could not see a performance difference above the normal jitter. Both cases take ~ 1:22 +/-1sec to run. The method is used during that test for ~200ms. Its caller, BoundSet.incorporate(), is dominated by another method reduceOneConstraint().
As far as i understand properTypesByInferenceVariable() is super fast on cases that would not require to compute it and would only take significant time if the result will be used.
image

Copy link
Contributor

@stephan-herrmann stephan-herrmann left a comment

Choose a reason for hiding this comment

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

I executed GenericsRegressionTest_1_8 with and without this PR and could not see a performance difference above the normal jitter. Both cases take ~ 1:22 +/-1sec to run. The method is used during that test for ~200ms. Its caller, BoundSet.incorporate(), is dominated by another method reduceOneConstraint().
As far as i understand properTypesByInferenceVariable() is super fast on cases that would not require to compute it and would only take significant time if the result will be used.

Just from that one screenshot I couldn't 100% follow your reasoning, but I appreciate you had a look into it.

@jukzi jukzi merged commit e498a7a into eclipse-jdt:master Feb 10, 2025
10 checks passed
@jukzi jukzi deleted the properTypesByInferenceVariable branch February 10, 2025 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants