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

fix: optimize splitOnWordBoundaries #999

Merged
merged 2 commits into from
Nov 17, 2023
Merged

fix: optimize splitOnWordBoundaries #999

merged 2 commits into from
Nov 17, 2023

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Nov 15, 2023

Issue #

n/a

Description of changes

First pass at optimizing splitOnWordBoundaries which was refactored in #975. Avoids re-computation of "is start of word" when we already know it isn't the start of a word.

Follows after the similar Rust PR: smithy-lang/smithy-rs#3140

Timings before and after for camel casing everything in all-names-test-output.csv:

// BEFORE
took 2634 milliseconds
took 1990 milliseconds
took 2211 milliseconds
took 1822 milliseconds
took 2038 milliseconds

// AFTER
took 487 milliseconds
took 392 milliseconds
took 353 milliseconds
took 352 milliseconds
took 346 milliseconds

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aajtodd aajtodd requested a review from a team as a code owner November 15, 2023 18:46
0marperez
0marperez reviewed Nov 15, 2023
View reviewed changes
...ithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/utils/CaseUtils.kt Outdated
@@ -14,11 +16,12 @@ fun String.splitOnWordBoundaries(): List<String> {
// but this has some edge cases it doesn't handle well
val out = mutableListOf<String>()
// These are whole words but cased differently, e.g. `IPv4`, `MiB`, `GiB`, `TtL`
Copy link
Contributor

@0marperez 0marperez Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: this comment could be deleted or be moved up to completeWords

lauzadis
lauzadis approved these changes Nov 16, 2023
View reviewed changes
.changes/e8825e61-8155-4da0-a9c9-eae9a0d786d4.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not user-facing, I'd say it doesn't need a changelog

@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aajtodd aajtodd added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Nov 16, 2023
@aajtodd aajtodd merged commit 314264c into main Nov 17, 2023
13 of 14 checks passed
@aajtodd aajtodd deleted the optimize-naming-perf branch November 17, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianbotsf ianbotsf ianbotsf approved these changes

@0marperez 0marperez 0marperez approved these changes

@lauzadis lauzadis lauzadis approved these changes

Assignees
No one assigned
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aajtodd @lauzadis @0marperez @ianbotsf