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

flatten: use loop and ArrayDeque instead of recursion #1801

Merged
merged 16 commits into from
Mar 29, 2024
Merged

flatten: use loop and ArrayDeque instead of recursion #1801

merged 16 commits into from
Mar 29, 2024

Conversation

hoc081098
Copy link
Contributor

@hoc081098 hoc081098 commented Mar 1, 2024 •
edited
Loading

maybe fix #1794

@hoc081098 hoc081098 changed the title flatten: use loop and ArrayDequene instead of recursion flatten: use loop and ArrayDeque instead of recursion Mar 1, 2024
@arnaudgiuliani arnaudgiuliani self-requested a review March 1, 2024 08:25
@arnaudgiuliani
Copy link
Member

Could run some benchmark test to try to ensure we don't reproduce the same @hoc081098 ? 🙏

@arnaudgiuliani arnaudgiuliani mentioned this pull request Mar 1, 2024
Closed
@arnaudgiuliani arnaudgiuliani added core type:improvement Improving a current feature labels Mar 1, 2024
@hoc081098
Copy link
Contributor Author

Could run some benchmark test to try to ensure we don't reproduce the same @hoc081098 ? 🙏

Yes, I will add it 😊

@hoc081098
Copy link
Contributor Author

@arnaudgiuliani I have added some benchmarks.

BenchmarkClass.flatten            avgt   10  146.838 ± 15.775  ms/op
BenchmarkClass.newFlatten         avgt   10    0.880 ±  0.018  ms/op
  • BenchmarkClass.flatten uses flatten of current Koin 3.5.3.
  • BenchmarkClass.newFlatten uses the new implementation of flatten.

Could you check it again 🙏 ?

marcellogalhardo
marcellogalhardo approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@marcellogalhardo marcellogalhardo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job. I was planning to take on this issue, but you were faster! Your implementation is exactly what I had in mind, so thank you for contributing with the improvement. ☺️

examples/jvm-perfs/benchmark.txt Outdated Show resolved Hide resolved
examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Show resolved Hide resolved
examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Outdated
}

@OptIn(KoinInternalApi::class)
internal fun newFlatten(modules: List<Module>): Set<Module> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: can't you use the original function here instead of duplicating it?

projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated Show resolved Hide resolved
projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated Show resolved Hide resolved
projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated Show resolved Hide resolved
@arnaudgiuliani arnaudgiuliani changed the base branch from main to 3.6.0 March 4, 2024 13:51
@arnaudgiuliani arnaudgiuliani changed the base branch from 3.6.0 to main March 4, 2024 13:51
@arnaudgiuliani
Copy link
Member

Good work. I need to redirect it on 3.6 branch ... will do a merge from main to 3.6

@arnaudgiuliani arnaudgiuliani added the status:accepted accepted to be developed label Mar 4, 2024
@arnaudgiuliani arnaudgiuliani added this to the core-3.6.0 milestone Mar 4, 2024
@arnaudgiuliani
Copy link
Member

macos tests are fixed in another branch

@hoc081098
Copy link
Contributor Author

@arnaudgiuliani Should I change target branch to 3.6?

@hoangchungk53qx1
Copy link

:pray

@hoc081098 hoc081098 changed the base branch from main to 3.6.0 March 7, 2024 10:07
@hoc081098
Copy link
Contributor Author

@arnaudgiuliani please review it again 🙏

@arnaudgiuliani arnaudgiuliani merged commit 15dc82e into InsertKoinIO:3.6.0 Mar 29, 2024
3 of 4 checks passed
@arnaudgiuliani
Copy link
Member

@hoc081098 The optimization is really great but we have an issue as, the returned result can randomly not be in the same order as the module list.

flatten(m1, m2) can result in <m1,m2> or <m1,m2> which is breaking capacity to overload a definition, given the module order

@arnaudgiuliani
Copy link
Member

DefinitionOverrideTest is failing on branch 3.6.0

@hoc081098
Copy link
Contributor Author

DefinitionOverrideTest is failing on branch 3.6.0

@arnaudgiuliani
I think because I have used HashSet, I will create a PR to fix it

@hoangchungk53qx1
Copy link

hoangchungk53qx1 commented Apr 5, 2024 via email

@hoc081098
Copy link
Contributor Author

I think so, linkedhashmap is better than not.

Vào 9:22 Th 6, 5 thg 4, 2024 Petrus Nguyễn Thái Học <
@.***> đã viết:

DefinitionOverrideTest is failing on branch 3.6.0

I think because I have used HashMap, I will create a PR to fix it

—
Reply to this email directly, view it on GitHub
#1801 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AMNXWG25SEWZA6WWKEECBIDY3YDIFAVCNFSM6AAAAABEBBFOQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZYGY2DAOJUGY
.
You are receiving this because you commented.Message ID:
@.***>

🙏🙇

@hoc081098
Copy link
Contributor Author

@arnaudgiuliani please review PR #1841.
Many thanks 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcellogalhardo marcellogalhardo marcellogalhardo approved these changes

@arnaudgiuliani arnaudgiuliani arnaudgiuliani approved these changes

Assignees
No one assigned
Labels
core status:accepted accepted to be developed type:improvement Improving a current feature
Projects
None yet
Milestone
core-3.6.0
Development

Successfully merging this pull request may close these issues.

[koin-core] flatten function
4 participants
@hoc081098 @arnaudgiuliani @hoangchungk53qx1 @marcellogalhardo