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

improve RegistrationMacOsX performance #2245 #2267

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Sep 13, 2024

Pattern matching with String.split("-{80}\n") is quiet expensive especially if the string contains more then 80 dashes in a row.

#2245

Pattern matching with String.split("-{80}\n") is quiet expensive
especially if the string contains more then 80 dashes in a row.

eclipse-platform#2245
@jukzi jukzi requested a review from sratz September 13, 2024 11:37
Copy link
Contributor

github-actions bot commented Sep 13, 2024

Test Results

 1 815 files  ±0   1 815 suites  ±0   1h 30m 47s ⏱️ - 1m 40s
 7 697 tests ±0   7 469 ✅ +1  228 💤 ±0  0 ❌  - 1 
24 252 runs  ±0  23 503 ✅ +1  749 💤 ±0  0 ❌  - 1 

Results for commit 4c67a49. ± Comparison against base commit 52470ac.

♻️ This comment has been updated with latest results.

Copy link
Member

@sratz sratz left a comment

Choose a reason for hiding this comment

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

Change looks good in principle.

However, there are exactly 80 dashes in that output, not more than 80.

Are you sure this will actually boost performance?

@jukzi
Copy link
Contributor Author

jukzi commented Sep 13, 2024

Are you sure this will actually boost performance?

I dont't have mac to test it locally. It's just that this split was logged so often that it can be seen as hotspot and local generic tests show that the alternative regex is faster, also reusing the splitted result may help

Copy link
Member

@sratz sratz left a comment

Choose a reason for hiding this comment

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

Ok, then let's merge this and see if it helps 👍

@jukzi jukzi merged commit 023f3de into eclipse-platform:master Sep 13, 2024
16 checks passed
@jukzi jukzi deleted the RegistrationMacOsX branch September 13, 2024 13:23
@BeckerWdf
Copy link
Contributor

BeckerWdf commented Sep 13, 2024

Are you sure this will actually boost performance?

I don't have mac to test it locally. It's just that this split was logged so often that it can be seen as hotspot and local generic tests show that the alternative regex is faster, also reusing the splitted result may help

There are JUnit tests that have "real-life" console outputs. See: TestUnitRegistrationMacOsX. These should be runnable on all OSes. Can you provide numbers how much faster they run?

@BeckerWdf
Copy link
Contributor

Thanks for taking care of performance.

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.

4 participants