Skip to content

flatten: use loop and ArrayDeque instead of recursion#1801

Merged
arnaudgiuliani merged 16 commits into
InsertKoinIO:3.6.0from
hoc081098:hoc081098-patch-1
Mar 29, 2024
Merged

flatten: use loop and ArrayDeque instead of recursion#1801
arnaudgiuliani merged 16 commits into
InsertKoinIO:3.6.0from
hoc081098:hoc081098-patch-1

Conversation

@hoc081098

@hoc081098 hoc081098 commented Mar 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

arnaudgiuliani commented Mar 1, 2024

Copy link
Copy Markdown
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

hoc081098 commented Mar 1, 2024

Copy link
Copy Markdown
Contributor Author

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

Yes, I will add it 😊

@hoc081098

hoc081098 commented Mar 2, 2024

Copy link
Copy Markdown
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

@marcellogalhardo marcellogalhardo left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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. ☺️

Comment thread examples/jvm-perfs/benchmark.txt
Comment thread examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt
Comment thread examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Outdated
}

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

@marcellogalhardo marcellogalhardo Mar 4, 2024

Copy link
Copy Markdown
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?

Comment thread projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated
Comment thread projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated
Comment thread projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated
@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

arnaudgiuliani commented Mar 4, 2024

Copy link
Copy Markdown
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

arnaudgiuliani commented Mar 4, 2024

Copy link
Copy Markdown
Member

macos tests are fixed in another branch

marcellogalhardo
marcellogalhardo reviewed Mar 4, 2024
View reviewed changes
Comment thread examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Outdated
Comment thread examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Outdated
@hoc081098

hoc081098 commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

@arnaudgiuliani Should I change target branch to 3.6?

@hoangchungk53qx1

hoangchungk53qx1 commented Mar 5, 2024

Copy link
Copy Markdown

:pray

@hoc081098 hoc081098 changed the base branch from main to 3.6.0 March 7, 2024 10:07
hoc081098
hoc081098 commented Mar 7, 2024
View reviewed changes
Comment thread projects/core/koin-core/src/commonMain/kotlin/org/koin/core/module/Module.kt Outdated
hoc081098
hoc081098 commented Mar 7, 2024
View reviewed changes
Comment thread examples/jvm-perfs/src/main/kotlin/org/koin/benchmark/nestedModules.kt Outdated
@hoc081098

hoc081098 commented Mar 7, 2024

Copy link
Copy Markdown
Contributor Author

@arnaudgiuliani please review it again 🙏

@arnaudgiuliani arnaudgiuliani merged commit 15dc82e into InsertKoinIO:3.6.0 Mar 29, 2024
@arnaudgiuliani

arnaudgiuliani commented Apr 4, 2024

Copy link
Copy Markdown
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

arnaudgiuliani commented Apr 4, 2024

Copy link
Copy Markdown
Member

DefinitionOverrideTest is failing on branch 3.6.0

@hoc081098

hoc081098 commented Apr 5, 2024

Copy link
Copy Markdown
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

hoangchungk53qx1 commented Apr 5, 2024 via email

Copy link
Copy Markdown

@hoc081098

hoc081098 commented Apr 5, 2024

Copy link
Copy Markdown
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

hoc081098 commented Apr 5, 2024

Copy link
Copy Markdown
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

@arnaudgiuliani arnaudgiuliani arnaudgiuliani approved these changes

+1 more reviewer

@marcellogalhardo marcellogalhardo marcellogalhardo approved these changes

Reviewers whose approvals may not affect merge requirements

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