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

CLDR-7277 Parallel ConsoleCheck #3154

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Aug 4, 2023

about 2x improvement, could be improved even more especially when targetting batch (non-interactive) mode

CLDR-7277

  • This PR completes the ticket.

ALLOW_MANY_COMMITS=true

@srl295 srl295 self-assigned this Aug 4, 2023
Comment on lines +435 to +483
TestCache testCache = new TestCache(); // for parallelism
testCache.setFactory(cldrFactory, checkFilter);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the core change: use TestCache (same as ST! ) to manage the CheckCLDR instances.

This does have an added benefit that we're running more similar code in ConsoleCheck and ST… I don't anticipate divergence otherwise, but it does mean the environ is similar.

about 2x improvement, could be improved especially when targetting batch mode
- new option "-1" to request sequential console check
- runs in sequential mode under certain circumstances
- also, added CheckCLDR.toString() to just show a simple class name
@srl295 srl295 force-pushed the cldr-7277/parallel-consolecheck branch from 3996ea7 to 8e2ef1c Compare November 30, 2023 20:57
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • tools/cldr-code/src/main/java/org/unicode/cldr/test/CheckCLDR.java is now changed in the branch
  • tools/cldr-code/src/main/java/org/unicode/cldr/test/ConsoleCheckCLDR.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@srl295 srl295 changed the title CLDR-7277 hack for ConsoleCheck to parallel CLDR-7277 Parallel ConsoleCheck Nov 30, 2023
@srl295 srl295 requested a review from btangmu November 30, 2023 20:59
@srl295 srl295 marked this pull request as ready for review November 30, 2023 20:59
@srl295
Copy link
Member Author

srl295 commented Nov 30, 2023

should be feature-complete. i'm not sure how to use the html and example features, but it should call the same code as before.

@srl295
Copy link
Member Author

srl295 commented Dec 19, 2023

Hrm, this might conflict with CLDR-17289 somehow.

@btangmu
Copy link
Member

btangmu commented Dec 20, 2023

"might conflict with CLDR-17289 somehow" -- it shouldn't be a problematic merge conflict if that's what you mean -- only one file in common, CheckCLDR.java, and only a few lines

or do you mean a conflict in some other sense?

@srl295
Copy link
Member Author

srl295 commented Dec 20, 2023

"might conflict with CLDR-17289 somehow" -- it shouldn't be a problematic merge conflict if that's what you mean -- only one file in common, CheckCLDR.java, and only a few lines

or do you mean a conflict in some other sense?

Just being cautious. Somehow i thought this was merged but it wasn't.

This would be great to get in, brings a bit of a speedup.

@srl295 srl295 merged commit 2550a54 into unicode-org:main Dec 20, 2023
7 checks passed
@srl295 srl295 deleted the cldr-7277/parallel-consolecheck branch December 20, 2023 19:40

subtotalCount.clear();
if (CLDRFile.isSupplementalName(localeID)) return;
Copy link
Member

Choose a reason for hiding this comment

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

We should check to see if CLDRFile.isSupplementalName should be used anymore. It is a holdover from when the supplemental data files were in the same folder as the locale-based files (en.xml, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

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