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

Run processors on whole of text #16155

Merged
merged 5 commits into from
Jun 17, 2021

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 art27@cantab.net

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 <art27@cantab.net>
@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
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@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 <art27@cantab.net>
@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 <art27@cantab.net>
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 <art27@cantab.net>
@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.

None yet

6 participants