Skip to content

Fix quadratic decode time on the number of CustomUnmarshaler nodes - #967

Open
orivej wants to merge 2 commits into
goccy:masterfrom
orivej:customunmarshaler
Open

orivej wants to merge 2 commits into
goccy:masterfrom
orivej:customunmarshaler

Conversation

@orivej

@orivej orivej commented Oct 23, 2026 •

Copy link
Copy Markdown

Fixes #965
Closes #714 as obsolete

Please see individual commits.

This is a speed up, all tests pass.

The case of `ref.size == idx` is removed because after the introduction of addToken
insertToken is not used at the end. Otherwise it should just call addToken.
@codecov-commenter

codecov-commenter commented Oct 23, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.77%. Comparing base (edee2f9) to head (c90d5c3).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   80.65%   80.77%   +0.11%     
==========================================
  Files          22       22              
  Lines        6845     6839       -6     
==========================================
+ Hits         5521     5524       +3     
+ Misses        905      898       -7     
+ Partials      419      417       -2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chiliec chiliec left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed and tested locally: go test ./... passes, and I diffed decode results and the bytes passed to CustomUnmarshaler on master vs this branch for implicit-null/tag-default insertion at EOF and comment-heavy inputs — identical. With a custom unmarshaler it's 4.5 ms / 21 ms for 1k / 4k items (master: 473 ms / 9.3 s), and it scales linearly, unlike my #966. One note: insertToken no longer handles idx == size; nothing calls it that way today, but a guard that falls back to addToken would make that explicit. LGTM — I've closed #966 in favour of this.

@orivej
orivej force-pushed the customunmarshaler branch from 366bf7d to 4281383 Compare October 24, 2026 22:18
Fixes goccy#965 quadratic blow up in scanning the whole document just to count newlines in
the comments before the token.
@orivej
orivej force-pushed the customunmarshaler branch from 4281383 to c90d5c3 Compare October 24, 2026 22:20
Comment thread internal/format/format.go
for prev := tk.Prev; prev != nil && prev.Type == token.CommentType; prev = prev.Prev {
newlines += strings.Count(normalizeNewLineChars(prev.Origin), "\n")
}
return strings.Repeat("\n", newlines) + tk.Origin

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have checked that when newlines is 0 this is free (tk.Origin is returned without copying) and removed the special case (if newlines == 0 return tk.Origin).

Comment thread parser/context.go
ref.tokens = append(ref.tokens, tk)
ref.size = len(ref.tokens)
return
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The case of ref.size == idx is removed because after the introduction of addToken
insertToken is not used at the end. Otherwise it should just call addToken.

This branch has not been deployed

No deployments
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.

Decode time is quadratic on the number of CustomUnmarshaler nodes

3 participants