zoekt: 40% of CPU cycles spent on regexp.String

[keegancsmith]

https://console.cloud.google.com/profiler;timespan=4h;end=2024-03-27T19:47:49.196Z/zoekt-webserver/cpu;history=percentage,show:github%5C.com%2Fsourcegraph%2Fzoekt%5C.%5C~28%5C*indexData%5C~29%5C.List$?referrer=search&project=sourcegraph-dev

https://sourcegraph.slack.com/archives/C05RHQLMH2M/p1711569390014179

/cc @sourcegraph/search-platform

[keegancsmith]

The above graph indicates on march 12th we went from this being 0% to 60% of CPU. Checking our deployment logs that was the first time we deployed with a version compiled with go1.22. This is likely a regression related to that. Here are the commits that changed in that deployment as well, I don't see anything that would change the behaviour of how we construct a regexp matchtree:

$ git log --oneline 19fa44cea9eb...371a303692de
371a303 sourcegraph: Remove HTTP APIs (#738)
74bbcc9 ranking: allow partial overlap with symbol (#742)
27d9399 remove deprecated ShardMaxImportantMatch TotalMaxImportantMatch (#744)
1c158f9 Don't truncate file before detecting language (#740)
b227501 all: gofumpt -l -w .
245e0ce gomod: update otel and circl for CVEs (#737)
7ec3d8e gomod: update grpc for GHSA-m425-mq94-257g (#736)
0ddb91f all: use stdlib slices package (#735)
6201776 gomod: update mountinfo to latest (#734)
746f388 matchtree: fix panic for missing files (#733)
fdc144f all: gofmt -s -w . (#730)

[keegancsmith]

Alright it is almost certainly this change which is the root cause golang/go@98c9f27

It introduces the use of unicode.SimpleFold over all runes in your literal. As mentioned in the commit description this is supposed to be only used for debugging. However, we need it so we can convert a syntax.Regexp into a regexp.Regexp. It has always bothered me we went via .String, so will investigate if we can do something else.

[mmanela]

@keegancsmith What impact on the end-user would this have? Does it impact user perf or just is using too much CPU overall?

[keegancsmith]

This has a noticeable impact, but not quite as much as half as slow given that IO is often the bottleneck and CPU usage is bursty. But for example I just checked our continuous performance monitoring and you can notice a regression in the graphs when this was deployed to dotcom:

https://ui.honeycomb.io/sourcegraph/datasets/search/result/bLDGcUQRjgh

[keegancsmith]

Here is a temporary fix, just use a version of regexp.String with the slowdown reverted out of it: sourcegraph/zoekt#753

Bigger change: We rely on grafana/regexp but it hasn't been updated in 2 years. So I…

• forked it https://github.com/sourcegraph/regexp
• copied in the latest changes in go stdlib sourcegraph/regexp@0e43c23
• merged it into the speedup branch sourcegraph/regexp@83177f7

Now that we control the fork I started adding things I wish we could do:

• Convert stdlib syntax.Regexp into the forks version sourcegraph/regexp@2035a01
• Make it possible to create a regexp.Regexp from a syntax.Regexp without creating a string sourcegraph/regexp@9f5c11d

I want to finish up this change next week. But before I do that some other simplifications to zoekt:

• Remove our support for gob and net/rpc. This introduces lots of uses of syntax.Regexp which make it hard to interact.
• Possibly introduce an intermediate representation between query.Q and matchtree. Then it doesn't matter that Regexp.String is slower since we will only run it once per search instead of once per shard. This is bigger change, so might be better to just rely on our fork.

[keegancsmith]

Fairly confident the workaround in sourcegraph/zoekt#753 will solve this problem in the shortterm. My goal today related to this work is to:

• ship the change to sourcegraph.com
• monitor effects on the profiler
• file follow up issues (mostly related to future work in my last comment)
• close this.

The goal is that we don't ship this regression in the next release which is being cut tomorrow. This is why I am also marking this as a release blocker.

[keegancsmith]

• I filed https://github.com/sourcegraph/sourcegraph/issues/61524 to track the tech debt introduced to solve this problem
• profiler output on dotcom has syntax.Regexp.String nowhere to be seen.
• Look at this beautiful graph of CPU usage on dotcom for the last week (we need to scale down as well!)

metrics-explorer link