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.

Sign up for GitHub

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

Jump to bottom

Add NeedPostProcess for Parser interface to improve performance of csv parser and some external parser #15153

Merged
merged 1 commit into from
Apr 13, 2021
Merged

Add NeedPostProcess for Parser interface to improve performance of csv parser and some external parser #15153

merged 1 commit into from
Apr 13, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 25, 2021 •
edited
Loading

Before this PR, it took 15s to render a 900 lines csv file with 6 columns on my macOS pro 2020.
After this PR, it will spend about 250ms.

So PostProcess is very slow and it's useless for rendering csv file.

This PR also make some external markup tool faster to allow them ignore PostProcess.

@lunny lunny added the performance/speed performance issues with slow downs label Mar 25, 2021
@lunny lunny added this to the 1.15.0 milestone Mar 25, 2021
@lunny lunny changed the title Add NeedPostProcess for Parser interface to improve performance of csv render Add NeedPostProcess for Parser interface to improve performance of csv parser and some external parser Mar 25, 2021
@zeripath
Copy link
Contributor

disabling postprocess means you don't get sanitization

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 25, 2021
@lunny
Copy link
Member Author

lunny commented Mar 26, 2021 •
edited
Loading

disabling postprocess means you don't get sanitization

No. sanitization has still been kept. see https://github.com/go-gitea/gitea/pull/15153/files#diff-6e514d0fdd340f9204579f783328a26600737870a4ea70726b5b58ffbe5667f5R94

@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 Apr 11, 2021
@lunny lunny force-pushed the lunny/csv_render_performance branch from 22e32bc to 25207c2 Compare April 13, 2021 05:07
@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 Apr 13, 2021
@lunny lunny merged commit 66f0fd0 into go-gitea:master Apr 13, 2021
@lunny lunny deleted the lunny/csv_render_performance branch April 13, 2021 07:06
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 2021
@zeripath zeripath added the type/enhancement An improvement of existing functionality label Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@KN4CK3R KN4CK3R KN4CK3R approved these changes

Assignees
No one assigned
Labels
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/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lunny @zeripath @KN4CK3R @GiteaBot