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

NonEmptySet: change 2nd param of constructor to Iterable. OptimizenonEmptySetOf and toNonEmptySetOrNull #3389

Merged
merged 8 commits into from May 13, 2024
Merged

NonEmptySet: change 2nd param of constructor to Iterable. OptimizenonEmptySetOf and toNonEmptySetOrNull #3389

merged 8 commits into from May 13, 2024

Conversation

hoc081098
Copy link
Contributor

  • Change the second param of constructor to Iterable (instead of Set).
  • Avoid unnecessary copying in nonEmptySetOf and toNonEmptySetOrNull.

@hoc081098
optimize(NonEmptySet): change second param of constructor to `Iterabl…
1c64bdf 
…e` (instead of Set). Avoid unnecessary copying in `nonEmptySetOf` and `toNonEmptySetOrNull`
@serras
Copy link
Member

serras commented Mar 3, 2024

I am not really sure what is the optimization here. How are you ensuring that elements are not duplicated when using nonEmptySetOf?

Furthermore, note that we are soon going to make arrow-2 the new main branch. There NonEmptySet is defined differently, using an inline value class. It would be better if optimizations would be applied there, as we don't expect to make any further 1.2.x release.

@serras serras mentioned this pull request Mar 3, 2024
Closed
@hoc081098 hoc081098 changed the base branch from main to arrow-2 March 4, 2024 01:35
@hoc081098 hoc081098 changed the base branch from arrow-2 to main March 4, 2024 01:35
@hoc081098
Copy link
Contributor Author

I am not really sure what is the optimization here. How are you ensuring that elements are not duplicated when using nonEmptySetOf?

In constructor, we have used setOf(first) + rest, it returns a Set.

@hoc081098
Copy link
Contributor Author

hoc081098 commented Mar 4, 2024 •
edited

  • Prev code: res (varargs === Array) -> toSet -> setOf(first) + res uses 2 loops internally.

  • After code : res (varargs === Array) -> asList -> setOf(first) + res uses 1 loops internally (because asList does not copy the array to List, but returns a read-only wrapper)
    🙏

@hoc081098
Merge branch 'arrow-2' of https://github.com/arrow-kt/arrow into none…
59a3f39 
…mptyset-optimize

# Conflicts:
#	arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptySet.kt
#	arrow-libs/optics/arrow-optics-ksp-plugin/src/main/kotlin/arrow/optics/plugin/internals/processor.kt
@hoc081098 hoc081098 changed the base branch from main to arrow-2 March 4, 2024 07:36
hoc081098
hoc081098 commented Mar 4, 2024
View reviewed changes
.idea/vcs.xml Outdated Show resolved Hide resolved
hoc081098
hoc081098 commented Mar 4, 2024
View reviewed changes
.idea/vcs.xml Outdated Show resolved Hide resolved
@hoc081098
Copy link
Contributor Author

hoc081098 commented Mar 4, 2024 •
edited

  • Prev code: res (varargs === Array) -> toSet -> setOf(first) + res uses 2 loops internally.
  • After code : res (varargs === Array) -> asList -> setOf(first) + res uses 1 loops internally (because asList does not copy the array to List, but returns a read-only wrapper)
    🙏

I have changed the target branch to arrow-2 branch. @serras Please review it again 🙏

@nomisRev nomisRev added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Mar 14, 2024
serras
serras requested changes Mar 14, 2024
View reviewed changes
Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

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

These generalizations look quite useful. Thanks for the contribution!

arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptySet.kt Outdated Show resolved Hide resolved
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/NonEmptySet.kt Outdated Show resolved Hide resolved
hoc081098 and others added 2 commits March 16, 2024 15:46
@hoc081098 hoc081098 requested a review from serras March 16, 2024 08:58
@serras serras changed the base branch from arrow-2 to main May 12, 2024 19:16
@serras serras changed the base branch from main to arrow-2 May 12, 2024 19:17
@serras
Copy link
Member

serras commented May 12, 2024

@hoc081098 sorry for the unnecesary noise but, would it be possible for you to rebase these changes into the new main? Otherwise it's impossible for us to merge properly this.

@hoc081098 hoc081098 changed the base branch from arrow-2 to main May 13, 2024 08:42
@hoc081098 hoc081098 changed the base branch from main to arrow-2 May 13, 2024 08:42
@serras serras merged commit 08f64aa into arrow-kt:arrow-2 May 13, 2024
11 checks passed
@hoc081098 hoc081098 mentioned this pull request May 15, 2024
Merged
@hoc081098 hoc081098 deleted the nonemptyset-optimize branch May 15, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serras serras serras approved these changes

Assignees
No one assigned
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hoc081098 @serras @nomisRev