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

Fix metadata - alternate approach #419

Closed
wants to merge 1 commit into from

Conversation

maroux
Copy link

@maroux maroux commented Jul 9, 2024

Alternative approach to #418.

These lines added in #400 caused the problem since the result of multiple append calls can return two slices backed by the same backing array. See demo here: https://go.dev/play/p/2FGKuCvCUKz. The bug only surfaces under specific conditions when the capacity of parser.context is increased by more than 1 due to this append call. This PR removes the append calls that cause this issue, as well as optimizing some places where you don't need to append since the length is predetermined -- that is, instead of:

a = make([]string, 0, N)
for i := 0; i < N; i++ {
        a = append(a, value)
}

prefer:

a = make([]string, N)
for i := 0; i < N; i++ {
        a[i] = value
}

For the record, I think #418 is the better fix since it doesn't rely on ensuring that p.context = append(p.context, ...) is never called and thus more future-proof, however, I'm not sure why these lines were added - maybe performance - so can't be sure if people (cc @arp242) like the idea of removing it.

Fixes #417

@arp242
Copy link
Collaborator

arp242 commented Jul 9, 2024

For the record, I think #418 is the better fix

Yes I agree. Closing this. I don't think I added that for a hugely specific reason other than reading the code while fixing that bug and thinking "we can optimize this a bit".

@arp242 arp242 closed this Jul 9, 2024
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.

metadata.Keys (and thus metadata.Undecoded) is invalid in some cases
2 participants