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

Use multiplication instead of division by a constant #42

Merged
merged 1 commit into from
May 11, 2020
Merged

Use multiplication instead of division by a constant #42

merged 1 commit into from
May 11, 2020

Conversation

Bodigrim
Copy link
Collaborator

@Bodigrim Bodigrim commented May 8, 2020

Fixes #39.

Benchmark     Before    After
-------------------------------
NFD/Korean   22.55 ms  11.33 ms
NFKD/Korean  22.78 ms  11.06 ms
NFC/Korean   17.95 ms  10.72 ms
NFKC/Korean  18.00 ms  10.77 ms

harendra-kumar
harendra-kumar reviewed May 10, 2020
View reviewed changes
Copy link
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

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

Functionally, all looks good. I have mostly style related comments which you can ignore or fix. You can consider the last comment for change if perf remains the same.

Data/Unicode/Internal/Division.hs Outdated Show resolved Hide resolved
Data/Unicode/Internal/Division.hs Outdated Show resolved Hide resolved
@harendra-kumar
Copy link
Member

Results look great, we got 2x improvement. Let's merge this and then check how much we get with #37 .

@Bodigrim
Copy link
Collaborator Author

Ah, sorry for indentation mess-up. Should be fixed now.
You are right, there is no performance gain from using isDivisibleBy28, so I removed it.

@harendra-kumar
Copy link
Member

Looks good. You can squash, rebase, push/merge it to master.

@Bodigrim Bodigrim merged commit c1f2c27 into composewell:master May 11, 2020
@Bodigrim Bodigrim deleted the division branch May 11, 2020 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harendra-kumar harendra-kumar harendra-kumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Speed up division operation in hangul decomposition
2 participants
@Bodigrim @harendra-kumar