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

perf: only generate data-comment-url once #17618

Merged
merged 3 commits into from
Nov 14, 2021

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Nov 11, 2021

Data for diff with 2.5K additons 0 edits 0 deletions:

No-patch:
Split: 2.82 MB
Unified: 2.37 MB

Patch:
Split: 2.59 MB (-8.2%)
Unified: 2.15 MB (-9.3%)

- Resolves go-gitea#17167
- Don't generate the same `data-comment-url` over and over for each
edited LOC.
- Set `data-comment-url` for top-table of diff output.

Data for diff with 2.5K additons 0 edits 0 deletions:
```
No-patch:
Split: 2.82 MB
Unified: 2.37 MB

Patch:
Split: 2.59 MB (-8.2%)
Unified: 2.15 MB (-9.3%)
```
@silverwind
Copy link
Member

silverwind commented Nov 12, 2021

I don't quite understand why this has such a big performance impact. I thought variables like .Issue.HTMLURL are generated once and then the result is (cheaply) substituted into the template. Why does it have such a big impact?

If there is significant computation involved when substituting vars in templates, we ought to apply this technique to a lot more variables.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 12, 2021
@Gusted
Copy link
Contributor Author

Gusted commented Nov 13, 2021

I don't quite understand why this has such a big performance impact. I thought variables like .Issue.HTMLURL are generated once and then the result is (cheaply) substituted into the template. Why does it have such a big impact?

If there is significant computation involved when substituting vars in templates, we ought to apply this technique to a lot more variables.

I think, I've missed the nail by naming this PR - it's a performance improvement in the sense that the size that's being sent to the client is smaller as the repeated attribute is not in each new LOC, but instead only sent once into a top-element.

@lafriks lafriks added topic/ui Change the appearance of the Gitea UI skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Nov 13, 2021
@lafriks lafriks added this to the 1.16.0 milestone Nov 13, 2021
@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 Nov 13, 2021
@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 13, 2021
@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 Nov 13, 2021
@wxiaoguang
Copy link
Contributor

LGTM. More thoughts:

  1. Can data-path="{{$file.Name}} be moved to out like data-comment-url?
  2. Can {{svg "octicon-plus"}} be improved, too?

@silverwind
Copy link
Member

Can {{svg "octicon-plus"}} be improved, too?

It likely can not without <use> which we previously had but we dropped it because it doesn't work cross-origin, so I think the current SVG rendering solution is optimal.

@techknowlogick techknowlogick merged commit 8eddb75 into go-gitea:main Nov 14, 2021
@Gusted Gusted deleted the perf-data-comment branch November 16, 2021 00:55
Gusted added a commit to Gusted/gitea that referenced this pull request Nov 16, 2021
- Don't sent it with each line, instead send it at the top-element for
each file.
- Related:
go-gitea#17618 (comment)

2.5K Additions:
No-Patch:
Unified: 2.14 MB (2.14 MB size)
Split: 2.59 MB (2.59 MB size)

Patch:
Unified: 2.10 MB (2.10 MB size) (-1.8%)
Split: 2.55 MB (2.55 MB size) (-1.5%)
wxiaoguang pushed a commit that referenced this pull request Nov 19, 2021
- Don't sent it with each line, instead send it at the top-element for each file.
- Related:
#17618 (comment)

2.5K Additions:
No-Patch:
Unified: 2.14 MB (2.14 MB size)
Split: 2.59 MB (2.59 MB size)

Patch:
Unified: 2.10 MB (2.10 MB size) (-1.8%)
Split: 2.55 MB (2.55 MB size) (-1.5%)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
- Don't sent it with each line, instead send it at the top-element for each file.
- Related:
go-gitea#17618 (comment)

2.5K Additions:
No-Patch:
Unified: 2.14 MB (2.14 MB size)
Split: 2.59 MB (2.59 MB size)

Patch:
Unified: 2.10 MB (2.10 MB size) (-1.8%)
Split: 2.55 MB (2.55 MB size) (-1.5%)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate new-comment-url once instead of n times for PRs
6 participants