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.

Sign up for GitHub

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

Jump to bottom

ICU-22848 Add test for rbbi rule builder failure. #3089

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ICU-22848 Add test for rbbi rule builder failure. #3089

wants to merge 1 commit into from

Conversation

FrankYFTang
Copy link
Contributor

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22848
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang
Copy link
Contributor Author

@aheninger could you take a look at the test case I added. It is now hang. I think it is super slow

@aheninger
Copy link
Contributor

Looks like the state table builder is running in exponential time on the number of trailing dots in the test pattern, ".*X..................;"

This is deep in the guts of the dragon book state table construction algorithm. I doubt there is an easy fix - the algorithm is tricky and hard to fully understand.

I recommend just leaving this as a known issue. It's not a problem in actual real break rules.

@FrankYFTang FrankYFTang added the incomplete Needs work; do not approve/merge as is. label Aug 7, 2024
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/rbbitst.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
ICU-22848 Show the problem of infinity loop
7644adc 
Add test to show very slow creation
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/intltest/rbbitst.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

aheninger
aheninger requested changes Aug 12, 2024
View reviewed changes
icu4c/source/test/intltest/rbbitst.cpp
@@ -6133,6 +6134,13 @@ void RBBITest::TestBug22636() {
assertEquals(WHERE, ec, U_ZERO_ERROR);
}

void RBBITest::TestBug22848() {
if (quick || logKnownIssue("ICU-22848", "Very slow case")) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend

  • reduce the number of dots in the pattern to the minimum that causes the failure (copy from bug report comment). This will dramatically speed up the test.
  • add an AssertSuccess after creating the bi. So the test will actually fail, and not just hang.
  • re-title the PR to something like "Add test for rbbi rule builder failure."

The test should fail -- when the state table max size is exceeded the ec will return a failure.

@FrankYFTang FrankYFTang changed the title ICU-22848 Show the problem of infinity loop ICU-22848 Add test for rbbi rule builder failure. Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aheninger aheninger aheninger requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@aheninger aheninger

Labels
incomplete Needs work; do not approve/merge as is.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FrankYFTang @aheninger