Skip to content

Improve DocLineComparator performance #2575#2576

Draft
mehmet-karaman wants to merge 1 commit intoeclipse-platform:masterfrom
mehmet-karaman:DocLineComparator_2575
Draft

Improve DocLineComparator performance #2575#2576
mehmet-karaman wants to merge 1 commit intoeclipse-platform:masterfrom
mehmet-karaman:DocLineComparator_2575

Conversation

@mehmet-karaman
Copy link
Contributor

@mehmet-karaman mehmet-karaman commented Mar 16, 2026

Add a lightweight preliminary check before invoking String.equals(), which avoids unnecessary overhead while preserving correctness for other comparison cases.

Fixes #2575

@mehmet-karaman
Improve DocLineComparator performance eclipse-platform#2575
85a4a63 
Add a lightweight preliminary check before invoking String.equals(),
which avoids unnecessary overhead while preserving correctness
for other comparison cases.

Fixes eclipse-platform#2575
@mehmet-karaman mehmet-karaman marked this pull request as draft March 16, 2026 13:01
@mehmet-karaman
Copy link
Contributor Author

mehmet-karaman commented Mar 16, 2026 •
edited
Loading

Changed PR to draft, need to fix the test failures org.eclipse.compare.tests

@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

Test Results

    51 files   -  3      51 suites   - 3   35m 17s ⏱️ - 2m 46s
 4 525 tests  - 21   4 492 ✅  - 31   23 💤 ±0  10 ❌ +10 
12 171 runs   - 63  11 982 ✅  - 93  159 💤 ±0  30 ❌ +30 

For more details on these failures, see this check.

Results for commit 85a4a63. ± Comparison against base commit 4f46e73.

This pull request removes 21 tests. 
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testFileModificationValidator
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testGetProviderById
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testGetProviderGeneric
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testIsShared
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testLinkFailure
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testLinkSuccess
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testMapFailure
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testMapSuccess
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testMoveDeleteHook
org.eclipse.team.tests.core.RepositoryProviderTests ‑ testMoveDeleteHookBetweenProjects
…

iloveeclipse
iloveeclipse reviewed Mar 16, 2026
View reviewed changes
team/bundles/org.eclipse.compare/compare/org/eclipse/compare/internal/DocLineComparator.java
return linesToCompare[0].equals(linesToCompare[1]);
String s1 = linesToCompare[0];
String s2 = linesToCompare[1];
if (s1.isEmpty() == s2.isEmpty()) {
Copy link
Member

@iloveeclipse iloveeclipse Mar 16, 2026

Choose a reason for hiding this comment

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

So two non empty lines are equal?

Copy link
Contributor Author

@mehmet-karaman mehmet-karaman Mar 16, 2026

Choose a reason for hiding this comment

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

How it comes? The first and the second check is just to avoid index out of bounds exceptions..

Copy link
Member

@iloveeclipse iloveeclipse Mar 16, 2026

Choose a reason for hiding this comment

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

Put a breakpoint in the return true new line and run DocLineComparatorTest

Copy link
Contributor Author

@mehmet-karaman mehmet-karaman Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It is supposed to work only on the positive case :) Going to check this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iloveeclipse iloveeclipse iloveeclipse left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DocLineComparator performance issue

2 participants

@mehmet-karaman @iloveeclipse