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

Add Value creation methods to allow marshaling #21

Closed
wants to merge 5 commits into from

Conversation

vincent-163
Copy link

I was looking for a dynamic json parser without generating go structs and found this awesome project. It also has good performance as a plus.
It seems like MarshalTo has just been implemented, which makes it possible to create json values.
There are many advantages using the same library for marshaling and unmarshaling, including reusing parts of a json object to create another json object, fewer dependencies. etc.
MarshalTo already provides a sound basis for marshaling, and here's one more piece of code to make it work.
I'll try to stay consistent with other parts of the code but correct me if necessary.
Thanks again for your hard work.

@codecov
Copy link

codecov bot commented Jan 18, 2019

Codecov Report

Merging #21 into master will increase coverage by 0.35%.
The diff coverage is 97.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   92.78%   93.14%   +0.35%     
==========================================
  Files           8        9       +1     
  Lines         970     1050      +80     
==========================================
+ Hits          900      978      +78     
- Misses         48       50       +2     
  Partials       22       22
Impacted Files Coverage Δ
parser.go 90.31% <ø> (ø) ⬆️
gen.go 97.5% <97.5%> (ø)

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 f1ad9dd...a37952c. Read the comment docs.

gen.go Outdated
// NewObject returns a new Value with the parameter as its initial content.
func (p *Parser) NewObject(m map[string]*Value) *Value {
o := p.c.getValue()
o.reset()
Copy link
Owner

Choose a reason for hiding this comment

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

There is no need in reset call, since getValue already clears the value.

"strconv"
)

// NewObject returns a new Value with the parameter as its initial content.
Copy link
Owner

Choose a reason for hiding this comment

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

It should be mentioned in the docs that the returned object is valid only until the next call to Parser.Parse.
The same applies to all the other Parser.New* functions.

Copy link
Owner

Choose a reason for hiding this comment

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

Also I'm skeptical about Parser.New* functions, since:

  • It is easy to shoot in the foot by calling Parser.Parse on the Parser used for creating new objects.
  • Continuous calls to Parser.New* on the same parser would lead to memory leak - the parser would hold more and more objects.

I'd just leave New* functions without tying them to the parser and for each New* function would add the corresponding Put*, which could be used for returning the object to a pool in order to reduce memory allocations.

Copy link
Author

Choose a reason for hiding this comment

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

The reason for Parser.New* functions is that it feels more consistent with existing APIs. You use one parser for parsing a single object, so for creating a single object. Maybe let the user do a Parser.Reset() before creating values, just like how the user parses an object before consuming it.

Copy link
Author

Choose a reason for hiding this comment

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

BTW I don't think using a Pool would help in this case. Value is a fixed-size object and it doesn't contain any interesting references (it's unlikely that the array/kv slices will be reused). Go's built-in memory manager and GC handles it well.

@valyala
Copy link
Owner

valyala commented Jan 21, 2019

@HEWENYANG , I re-worked the PR into Arena. See the corresponding commit ecda198 (this is release v1.4.0).

Thanks for the idea!

@valyala valyala closed this Jan 21, 2019
@vincent-163
Copy link
Author

Yeah, that looks much better. Thanks!
There are still some APIs that I find difficult to use though, and I'll cover them in another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants