flier / gohs

GoLang Binding of HyperScan https://www.hyperscan.io/
Other
280 stars 51 forks source link

Add handle implementation that does not depend on cgo.Handle #62

Closed martinmr closed 1 week ago

martinmr commented 1 week ago

cgo.Handle uses a sync.Map, which leads to large amounts of lock contention in highly concurrent use cases. Since 1.21, the Pinner object in the runtime provides similar functionality.

This change provides a new Pinner-based implementation of the Handle type to avoid lock-contention altogether.

martinmr commented 1 week ago

A second attempt at this after https://github.com/flier/gohs/pull/61

The background is that we have a service that does lots of regexes and is going very slowly under some workloads. We noticed that the profiles indicate most of the time is spent in lock contention when getting handles.

I found this reddit thread: https://www.reddit.com/r/golang/comments/1eqa11u/optimizing_cgo_handles/

The author found a way to optimize the cgo implementation, but his change has stalled in code review. However, the first comment shows a way to implement this without changes to the runtime using the new Pinner feature in the runtime package. Here's their example: https://gitlab.com/pygolo/py/-/blob/main/go-handle.go

Most of my change is based on that. Only changes are to make it work with the existing APIs.

I fixed the bugs and made sure that previous versions of go still work (tested with 1.18 because it was required by the test framework).

I deployed a version of our service with this library and performance immediately improved after the new version was live at around 8:30 in the graph below.

Screenshot 2024-10-17 at 8 42 43 PM

martinmr commented 1 week ago

There's still a lot of contention with this implementation in very concurrent cases, so I am closing this for now. Experimenting with the implementation from the blog post. Performance looks better for now but we'll see.

flier commented 6 days ago

Very interesting, let's see if we can optimize it