Skip to content

[refactor] mailIssueCommentToParticipants #10478

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

Closed
wants to merge 11 commits into from

Conversation

6543
Copy link
Member

@6543 6543 commented Feb 26, 2020

utilize map

@6543 6543 force-pushed the refactor-mail-notify branch from 0f78df4 to 9faf2e3 Compare February 26, 2020 14:52
@6543
Copy link
Member Author

6543 commented Feb 26, 2020

blocked by #10475 (if this is merged first a backport would be harder ...)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 26, 2020
@guillep2k
Copy link
Member

I don't know. I carefully considered maps for this when I did it. Every time you access a map entry a hash function is executed and possibly a bucket must be allocated. Maps are slower to populate than slices. IMHO the current implementation is faster because it checks the map only one time per item after it is allocated with the required size. Especially in the 90% of the cases where the list is quite short. I've even did some benchmarks, I believe.

I could be looking at this all wrong, though. 😅

@6543 6543 force-pushed the refactor-mail-notify branch from e71d683 to 0754971 Compare February 27, 2020 12:29
@6543
Copy link
Member Author

6543 commented Feb 27, 2020

I don't know. I carefully considered maps for this when I did it. Every time you access a map entry a hash function is executed and possibly a bucket must be allocated. Maps are slower to populate than slices. IMHO the current implementation is faster because it checks the map only one time per item after it is allocated with the required size. Especially in the 90% of the cases where the list is quite short. I've even did some benchmarks, I believe.

I could be looking at this all wrong, though.

we get rid of https://github.com/go-gitea/gitea/pull/10478/files#diff-959cf0e72f197b979a8047af5f475157L113-L119 witch is an impruvement in my opinion ... but how did you make the benchmarks?

@guillep2k
Copy link
Member

we get rid of https://github.com/go-gitea/gitea/pull/10478/files#diff-959cf0e72f197b979a8047af5f475157L113-L119 witch is an impruvement in my opinion ... but how did you make the benchmarks?

Those two loops run through the data exactly one time; you may think they go over the same items several times, but they don't. A for ... range on a map would be slower than that.

As for testing, I don't recall exactly, but there's only one way I'd do it, and it's programming loops with static data and timing them running in the 100,000 of times. The database must be excluded from the benchmarks because it's a common part of and the slowest by several orders of magnitude, of course.

@6543
Copy link
Member Author

6543 commented Feb 27, 2020

so if map dont add speed to it, I'll close this ... ?

@guillep2k
Copy link
Member

I don't know, others might have a different opinion than me. Or consider your code better for other reasons (e.g. clarity).

@6543
Copy link
Member Author

6543 commented Apr 6, 2020

this got it's change

@6543 6543 closed this Apr 6, 2020
@6543 6543 deleted the refactor-mail-notify branch April 6, 2020 17:02
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants