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

Support Go-JSON #14914

Closed
wants to merge 10 commits into from
Closed

Support Go-JSON #14914

wants to merge 10 commits into from

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 6, 2021

This PR provides a mechanism to select the JSON library at start-up, on configuration and at compile time.
Go-JSON support is also added.

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

@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 6, 2021
@zeripath zeripath added this to the 1.14.0 milestone Mar 6, 2021
@zeripath

This comment has been minimized.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 6, 2021
@zeripath zeripath removed this from the 1.14.0 milestone Mar 6, 2021
@a1012112796

This comment has been minimized.

@lafriks lafriks added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Mar 7, 2021
@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@zeripath

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@6543

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@zeripath

This comment has been minimized.

@silverwind

This comment has been minimized.

@silverwind silverwind added performance/speed performance issues with slow downs pr/wip This PR is not ready for review labels Apr 7, 2021
@zeripath
Copy link
Contributor Author

zeripath commented May 9, 2021

0.5.0 is out so I'll try to update this again at some point

@silverwind

This comment has been minimized.

@silverwind

This comment has been minimized.

@zeripath zeripath added type/enhancement An improvement of existing functionality and removed type/refactoring Existing code has been cleaned up. There should be no new functionality. pr/wip This PR is not ready for review labels Oct 21, 2021
@zeripath
Copy link
Contributor Author

OK I've updated this and made the JSON library selectable.

Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@silverwind
Copy link
Member

Seems a bit overkill to me to have it runtime-selectable, but I guess we can just remove that option once we settle on a module.

Is there an easy way to have at least a subset of tests run on all implementations?

@lunny
Copy link
Member

lunny commented Oct 22, 2021

I also think the end users should not need to know how to chose a json encoder/decoder. It's could be a build tag but not a config item.

@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 Jan 20, 2022
@techknowlogick techknowlogick added this to the 1.17.0 milestone Jan 20, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 20, 2022

I also have the question why do we need to make JSON library configurable. There might be other configurable libraries like YAML, INI, ORM ....

Does it bring more benefits than the complexity?

Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Sorry I will against make it configurable for end users. It should be a build tag.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 20, 2022

Sorry I will against make it configurable for end users. It should be a build tag.

I would not even introduce a build tag. If I find something is good enough, I will just use it to replace the old.

If there is no real benefit, we do not need to introduce too many options/tags.

@stale
Copy link

stale bot commented Apr 28, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 28, 2022
@stale stale bot removed the issue/stale label Apr 28, 2022
@lunny lunny removed this from the 1.17.0 milestone May 16, 2022
@silverwind
Copy link
Member

silverwind commented Mar 16, 2023

Just checking the issue tracker of go-json, it still has crash bugs.

I suggest sticking to encoding/json, as it will receive much more developer attention than those unstable/abandoned hobby projects and is bound to only get faster over time.

@lunny
Copy link
Member

lunny commented Mar 17, 2023

Just checking the issue tracker of go-json, it still has crash bugs.

I suggest sticking to encoding/json, as it will receive much more developer attention than those unstable/abandoned hobby projects and is bound to only get faster over time.

I suggest we can close this one and open a new when something is ready. @zeripath

@zeripath
Copy link
Contributor Author

zeripath commented Mar 18, 2023

When I last updated and fixed this PR go-json was working completely fine for Gitea - but the potential issue of there being bugs was a reason why I provided the configuration variable which @lunny did not like.

@silverwind we're already using json-iterator over encoding/json and I would note that encoding/json has not increased in speed significantly for the entire period time I have been involved in gitea.

However, it's clear that this PR is not going to be merged so I will close it.

@zeripath zeripath closed this Mar 18, 2023
@silverwind
Copy link
Member

silverwind commented Mar 18, 2023

I recommend dropping json-iterator as well. It has a number of mind-bogging bugs, just check their issue tracker. Gitea might not hit them currently, but it can in the future.

@zeripath
Copy link
Contributor Author

zeripath commented Mar 18, 2023

There is a considerable time penalty to using encoding/json over jsoniterator. You are more than welcome to write the PR and fix the multiple issues were we have relied on jsoniterator error interpretation (this PR has the fixes for those) and then do the performance tests to prove this.

IN FACT: Precisely this PR would have allowed for that test to be made and to allow us to consider dropping to encoding/json keep jsoniterator or try out go-json - without even re-compiling. And if it had turned out that jsoniterator/go-json were causing too many bugs support could have been simply dropped etc.

@silverwind
Copy link
Member

I guess we shouldn't try to fix what aint broken then.

On topic of benchmarks, I do find https://medium.com/geekculture/my-golang-json-evaluation-20a9ca6ef79c from 2021, so I guess you are right, encodings/json is worst:

image

@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. performance/speed performance issues with slow downs type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants