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 error to Decoder.PeekKind and add Decoder.More #146

Closed
wants to merge 1 commit into from

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Jan 24, 2025

Add an error return variable to PeekKind.
This avoids accidentally ignoring an error condition during peeking.

Add a Decoder.More method to compensate for loss of ergonomics. The More method can be used to quickly iterate through all elements or members of a JSON array or object.
This mirrors the existing jsonv1.Decoder.More method. Switch existing usages of PeekKind to use More.

Performance:

name                                           old time/op    new time/op    delta
Testdata/CanadaGeometry/Unmarshal/Concrete     1.20ms ± 1%    1.20ms ± 1%    ~     (p=0.310 n=5+5)
Testdata/CanadaGeometry/Unmarshal/Interface    1.64ms ± 0%    1.66ms ± 1%  +1.18%  (p=0.008 n=5+5)
Testdata/CitmCatalog/Unmarshal/Concrete        2.04ms ± 1%    2.13ms ± 0%  +4.37%  (p=0.008 n=5+5)
Testdata/CitmCatalog/Unmarshal/Interface       3.88ms ± 1%    4.02ms ± 1%  +3.63%  (p=0.008 n=5+5)
Testdata/GolangSource/Unmarshal/Concrete       5.51ms ± 0%    5.70ms ± 0%  +3.54%  (p=0.008 n=5+5)
Testdata/GolangSource/Unmarshal/Interface      9.09ms ± 1%    9.60ms ± 1%  +5.59%  (p=0.008 n=5+5)
Testdata/SyntheaFhir/Unmarshal/Concrete        3.60ms ± 1%    3.56ms ± 0%  -1.34%  (p=0.008 n=5+5)
Testdata/SyntheaFhir/Unmarshal/Interface       5.33ms ± 1%    5.40ms ± 1%  +1.26%  (p=0.016 n=5+5)
Testdata/TwitterStatus/Unmarshal/Concrete      1.16ms ± 1%    1.13ms ± 0%  -2.94%  (p=0.008 n=5+5)
Testdata/TwitterStatus/Unmarshal/Interface     2.02ms ± 1%    2.02ms ± 1%    ~     (p=0.421 n=5+5)

@dsnet
Copy link
Collaborator Author

dsnet commented Jan 24, 2025

I'm not sold that this is a good idea, but I wanted to see what it would take to make PeekKind return an error.

Add an error return variable to PeekKind.
This avoids accidentally ignoring an error condition during peeking.

Add a Decoder.More method to compensate for loss of ergonomics.
The More method can be used to quickly iterate through all
elements or members of a JSON array or object.
This mirrors the existing jsonv1.Decoder.More method.
Switch existing usages of PeekKind to use More.

Performance:

    name                                           old time/op    new time/op    delta
    Testdata/CanadaGeometry/Unmarshal/Concrete     1.20ms ± 1%    1.20ms ± 1%    ~     (p=0.310 n=5+5)
    Testdata/CanadaGeometry/Unmarshal/Interface    1.64ms ± 0%    1.66ms ± 1%  +1.18%  (p=0.008 n=5+5)
    Testdata/CitmCatalog/Unmarshal/Concrete        2.04ms ± 1%    2.13ms ± 0%  +4.37%  (p=0.008 n=5+5)
    Testdata/CitmCatalog/Unmarshal/Interface       3.88ms ± 1%    4.02ms ± 1%  +3.63%  (p=0.008 n=5+5)
    Testdata/GolangSource/Unmarshal/Concrete       5.51ms ± 0%    5.70ms ± 0%  +3.54%  (p=0.008 n=5+5)
    Testdata/GolangSource/Unmarshal/Interface      9.09ms ± 1%    9.60ms ± 1%  +5.59%  (p=0.008 n=5+5)
    Testdata/SyntheaFhir/Unmarshal/Concrete        3.60ms ± 1%    3.56ms ± 0%  -1.34%  (p=0.008 n=5+5)
    Testdata/SyntheaFhir/Unmarshal/Interface       5.33ms ± 1%    5.40ms ± 1%  +1.26%  (p=0.016 n=5+5)
    Testdata/TwitterStatus/Unmarshal/Concrete      1.16ms ± 1%    1.13ms ± 0%  -2.94%  (p=0.008 n=5+5)
    Testdata/TwitterStatus/Unmarshal/Interface     2.02ms ± 1%    2.02ms ± 1%    ~     (p=0.421 n=5+5)
@dsnet dsnet force-pushed the decoder-peek-error branch from 1b1c8ae to b7136a9 Compare January 24, 2025 23:53
@dsnet
Copy link
Collaborator Author

dsnet commented Jan 25, 2025

I posted my thoughts here: golang/go#63397 (reply in thread)

The more I look at it, the more unconvinced I am that PeekKind needs to return an error.

@mvdan
Copy link
Collaborator

mvdan commented Jan 27, 2025

I originally asked about PeekKind returning an error by looking at two particular use cases that would be improved by it, but from what you've written about other reasonable use cases, and the performance, overall I agree that I lean against adding an error return to PeekKind.

Like you however, I would improve the docs a bit so that they are clearer. I'd also like to see documented what PeekKind does when a non-EOF error is encountered, such as invalid syntax.

@dsnet
Copy link
Collaborator Author

dsnet commented Jan 27, 2025

Closing this for now. We can resurrect this in the future.

@dsnet dsnet closed this Jan 27, 2025
@dsnet dsnet deleted the decoder-peek-error branch January 27, 2025 18:44
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.

2 participants