Skip to content

Conversation

@zeripath
Copy link
Contributor

@zeripath zeripath commented Jun 14, 2021

There is an inefficiency in the design of our processors which means that Emoji
and other processors run in order n^2 time.

This PR forces all of the processors to process the entirety of text node before passing
back up. The fundamental inefficiency remains but it should be significantly
ameliorated.

Signed-off-by: Andrew Thornton [email protected]

There is an inefficiency in the design of our processors which means that Emoji
and other processors run in order n^2 time.

This PR forces the emoji processors to process the entirety of text node before passing
back up. The fundamental inefficiency remains but it should be significantly
ameliorated.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/bug performance/speed performance issues with slow downs backport/v1.14 labels Jun 14, 2021
@zeripath zeripath added this to the 1.15.0 milestone Jun 14, 2021
@lunny
Copy link
Member

lunny commented Jun 15, 2021

It's better to add a benchmark.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 15, 2021
zeripath added 2 commits June 15, 2021 19:57
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath force-pushed the run-emoji-processor-on-whole-of-data branch from 480c3c1 to c17207d Compare June 15, 2021 18:59
@zeripath zeripath changed the title Run emoji processors on whole of text Run processors on whole of text Jun 15, 2021
@zeripath
Copy link
Contributor Author

Benchmark added.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

142208659396 ns/op -> 1195548 ns/op

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 16, 2021
@zeripath
Copy link
Contributor Author

Things can be further improved by not having the processors reprocess text that they have already seen - i.e. track the next sibling and don't do anything till that happens. However that would require more changes to the processor.

@6543
Copy link
Member

6543 commented Jun 16, 2021

@zeripath this pull as is can be backported ... refactor should be done later :)

@codecov-commenter
Copy link

Codecov Report

Merging #16155 (fd12c9e) into main (f4d3bf7) will increase coverage by 0.03%.
The diff coverage is 73.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16155      +/-   ##
==========================================
+ Coverage   44.57%   44.60%   +0.03%     
==========================================
  Files         699      700       +1     
  Lines       82794    82925     +131     
==========================================
+ Hits        36906    36990      +84     
- Misses      39906    39946      +40     
- Partials     5982     5989       +7     
Impacted Files Coverage Δ
models/task.go 39.06% <ø> (ø)
modules/migrations/dump.go 0.00% <0.00%> (ø)
routers/api/v1/repo/migrate.go 44.17% <0.00%> (ø)
routers/web/user/task.go 0.00% <0.00%> (ø)
routers/api/v1/notify/user.go 11.62% <33.33%> (-13.38%) ⬇️
routers/api/v1/notify/notifications.go 45.16% <41.37%> (-54.84%) ⬇️
models/notification.go 65.78% <50.00%> (-0.30%) ⬇️
modules/emoji/emoji.go 81.15% <50.00%> (-1.92%) ⬇️
modules/migrations/migrate.go 28.78% <50.00%> (+0.55%) ⬆️
modules/git/repo_language_stats_nogogit.go 48.75% <66.66%> (+4.17%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffbf35b...fd12c9e. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 17, 2021
@zeripath zeripath merged commit 0db1048 into go-gitea:main Jun 17, 2021
@zeripath zeripath deleted the run-emoji-processor-on-whole-of-data branch June 17, 2021 10:35
zeripath added a commit to zeripath/gitea that referenced this pull request Jun 17, 2021
Backport go-gitea#16155

There is an inefficiency in the design of our processors which means that Emoji
and other processors run in order n^2 time.

This PR forces the processors to process the entirety of text node before passing
back up. The fundamental inefficiency remains but it should be significantly
ameliorated.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added the backport/done All backports for this PR have been created label Jun 17, 2021
6543 pushed a commit that referenced this pull request Jun 17, 2021
Backport #16155

There is an inefficiency in the design of our processors which means that Emoji
and other processors run in order n^2 time.

This PR forces the processors to process the entirety of text node before passing
back up. The fundamental inefficiency remains but it should be significantly
ameliorated.

Signed-off-by: Andrew Thornton <[email protected]>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
There is an inefficiency in the design of our processors which means that Emoji
and other processors run in order n^2 time.

This PR forces the processors to process the entirety of text node before passing
back up. The fundamental inefficiency remains but it should be significantly
ameliorated.

Signed-off-by: Andrew Thornton <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants