Replace the splitManifests regex

[borissmidt]

What this PR does / why we need it:
The regex is slow, we use helm in a gitops repo and template a lot of helm in our CI steps and it is getting quite slow because i've profiled helm itself and found true bottlenecks:

1. Splitting the yaml output (this part)
2. Sorting the yaml output because it has to parse the source
3. Reading the chart index in the chart downloader

Special notes for your reviewer:

If applicable:

• this PR contains user facing changes (the docs needed label should be applied if so)
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[mattfarina]

Reading the chart index in the chart downloader

FYI, you can get a massive performance boost by using JSON in the index.yaml. A JSON document is valid YAML. More recent versions of Helm detect JSON and use a JSON parser. JSON parsers are faster and use fewer resources. YAML is more difficult to parse due to things like anchors.

[TerryHowe]

I'd think there would be more test cases

[TerryHowe]

If I was writing this, I'd be tempted to look at creating a scanner and/or state machine to make this more readable. I think something could be done that wouldn't hurt performance.

[borissmidt]

Today i tried two implementations, one with a 'scanner/iterator' but this was clearer to read.

But then looking at the profiler output i saw that 'strings.Prefix' is highly optimized so i tried a 'naive' implementation which was actually twice as fast.

It now checks at very subslice that it start with "\n---".

[TerryHowe]

It is a lot easier to read now, thanks!

[borissmidt]

Yes it is still a draft pr, i will clean it up later but i wanted to know how the reaction would be. I'm not sure if a state machine would be simpler but i could try. There where not many tests to start with but i can add a few more.

…

On Thu, 14 Nov 2024, 16:27 Terry Howe, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/releaseutil/manifest.go <#13441 (comment)>: > + //if strings.HasPrefix(bigFile, "---") { + // i = 3 + //} Is this PR a WIP? ------------------------------ In pkg/releaseutil/manifest_test.go <#13441 (comment)>: > @@ -49,13 +50,99 @@ spec: image: fake-image cmd: fake-command` I'd think there would be more test cases ------------------------------ In pkg/releaseutil/manifest.go <#13441 (comment)>: > res[fmt.Sprintf(tpl, count)] = d count = count + 1 } return res } +func splitDocs(bigFile string) []string { + docs := make([]string, 0) + docStartIdx := 0 + docEndIdx := 0 + var char rune + var width int + // using a 'raw' for loop to avoid creating a complicated statemachine + i := 0 + //if strings.HasPrefix(bigFile, "---") { + // i = 3 + //} + + for i < len(bigFile) { If I was writing this, I'd be tempted to look at creating a scanner and/or state machine to make this more readable. I think something could be done that wouldn't hurt performance. — Reply to this email directly, view it on GitHub <#13441 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNXZFWO22R6LDQG72UNNGT2AS6MRAVCNFSM6AAAAABRUJETIWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMZWGMZDOMZXGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[borissmidt]

I've made an updated implementation which is simpler and more maintainable.

[TerryHowe]

You'll probably get more eyes on this if the DCO was fixed https://github.com/helm/helm/pull/13441/checks?check_run_id=33081873991

[borissmidt]

@TerryHowe thank you i've added the singing which fixes the DCO, I also rebased on master and improved the commti message. So i hope it is ready now for review.

[borissmidt]

Is there anything holding us back here?