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.

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

feature: improve performance if the element of the Value of *Model does not implement the AfterFindable interface. #799

Closed
seedeed opened this issue Nov 26, 2022 · 4 comments · Fixed by #805
Assignees
Labels
s: accepted This proposal was accepted. Someone can start working on it.
Milestone

Comments

@seedeed
Copy link

seedeed commented Nov 26, 2022

Description

I found that the afterFind method of *Model would create a log of goroutines via *errgroup.Group.Go(...) even if the element of the Value of *Model does not implement the AfterFindable interface, which might cause a lot of unneeded CPU cost. To avoid this, I think we could move the AfterFindable interface checking out of the Go function. If this's really a problem, I think I could submit a PR to fix it.

pop/callbacks.go

Lines 32 to 43 in 17f09c0

for i := 0; i < rv.Len(); i++ {
func(i int) {
wg.Go(func() error {
y := rv.Index(i)
y = y.Addr()
if x, ok := y.Interface().(AfterFindable); ok {
return x.AfterFind(c)
}
return nil
})
}(i)
}

Additional Information

No response

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

@github-actions github-actions bot added the stale label Dec 27, 2022
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

This issue was closed because it has been stalled for 30+7 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 3, 2023
@sio4 sio4 added s: triage Some tests need to be run to confirm the issue and removed stale s: closed labels Jan 6, 2023
@sio4 sio4 reopened this Jan 6, 2023
@sio4
Copy link
Member

sio4 commented Jan 13, 2023

Hi @seedeed, Thank you for reporting this issue. I agree with you, the block could spend unnecessary resources.

The block was recently touched with a new thing, and I opened a PR for your issue after that. The PR is #805, and the specific commit is 89774fa. Please take a look at the PR and/or the commit and give me feedback if the direction is the same as your idea, or please let me know your idea if you have another good way.

Thanks in advance!

@sio4 sio4 added this to the v6.1.1 milestone Jan 13, 2023
@sio4 sio4 self-assigned this Jan 13, 2023
@sio4 sio4 added s: accepted This proposal was accepted. Someone can start working on it. and removed s: triage Some tests need to be run to confirm the issue labels Jan 13, 2023
@sio4 sio4 closed this as completed in #805 Jan 18, 2023
@sio4
Copy link
Member

sio4 commented Jan 18, 2023

Hi @seedeed
I just merged my PR into the main. If you have any idea about it, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: accepted This proposal was accepted. Someone can start working on it.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants