Issue 44150043: code review 44150043: regexp: use sync.Pool (Closed)
Descriptionregexp: use sync.Pool
For machines, not threads.
Update Issue 4720
Patch Set 1 #Patch Set 2 : diff -r 5002ec2f1082 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 5002ec2f1082 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r afe351cefb6f https://go.googlecode.com/hg/ #MessagesTotal messages: 12
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=198bdc0984dd *** regexp: use sync.Pool For machines, not threads. Update Issue 4720 R=golang-dev, rsc CC=golang-dev https://codereview.appspot.com/44150043
Sign in to reply to this message.
Message was sent while issue was closed.
Wait, can this pool be made global now?
Sign in to reply to this message.
On Thu, Dec 19, 2013 at 2:29 AM, <dvyukov@google.com> wrote: > Wait, can this pool be made global now? > No. The machine is sized according to the regexp it can execute. Because it's not clear, I want to explain for the record why I accepted this use of sync.Pool for the free list inside the Regexp. Many Regexps are globals, created by regexp.MustCompile and used by multiple goroutines. If many goroutines hit a Regexp at the same time, each gets its own machine, and then they all end up in the free list for next time. If there is no next time, or next time is a long time from now, that memory is pinned unnecessarily. Using a sync.Pool lets the runtime reclaim that memory when it makes sense to do so. Regexp is unusual here because it is intended for use by multiple goroutines simultaneously, which makes the size of the free list unbounded (# of goroutines) and means that many Regexps do not have a defined lifetime. These are the problems that sync.Pool solves. Russ
Sign in to reply to this message.
On Thu, Dec 19, 2013 at 8:33 AM, Russ Cox <rsc@golang.org> wrote: > On Thu, Dec 19, 2013 at 2:29 AM, <dvyukov@google.com> wrote: > >> Wait, can this pool be made global now? >> > > No. The machine is sized according to the regexp it can execute. > > Because it's not clear, I want to explain for the record why I accepted > this use of sync.Pool for the free list inside the Regexp. > > Many Regexps are globals, created by regexp.MustCompile and used by > multiple goroutines. If many goroutines hit a Regexp at the same time, each > gets its own machine, and then they all end up in the free list for next > time. If there is no next time, or next time is a long time from now, that > memory is pinned unnecessarily. Using a sync.Pool lets the runtime reclaim > that memory when it makes sense to do so. Regexp is unusual here because it > is intended for use by multiple goroutines simultaneously, which makes the > size of the free list unbounded (# of goroutines) and means that many > Regexps do not have a defined lifetime. These are the problems that > sync.Pool solves. > Good to hear we're on the same page. I thought the same thing. My sending the encoding/gob change was a mistake. I didn't know that code well and was thinking it worked more like the (new) encoding/json per-type encoders.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/12/19 16:33:40, rsc wrote: > On Thu, Dec 19, 2013 at 2:29 AM, <mailto:dvyukov@google.com> wrote: > > > Wait, can this pool be made global now? > > > > No. The machine is sized according to the regexp it can execute. I see. I guess it does not make sense to have a global array of Pools for different "size classes". The remaining concern is: Are there cases where a program creates lots of local short-lived regexps (one-shot), and it's not possible to replace them with global ones? For this case we've introduced a global contended mutex.
Sign in to reply to this message.
On Fri, Dec 20, 2013 at 1:18 AM, <dvyukov@google.com> wrote: > On 2013/12/19 16:33:40, rsc wrote: > > On Thu, Dec 19, 2013 at 2:29 AM, <mailto:dvyukov@google.com> wrote: >> > > > Wait, can this pool be made global now? >> > >> > > No. The machine is sized according to the regexp it can execute. >> > > I see. > > I guess it does not make sense to have a global array of Pools for > different "size classes". > > The remaining concern is: > Are there cases where a program creates lots of local short-lived > regexps (one-shot), and it's not possible to replace them with global > ones? For this case we've introduced a global contended mutex. > Yes, that happens in any case where a program computes the regexp at run time. That's a bit unfortunate. Maybe this should be rolled back. Brad, was there any performance improvement here? Russ
Sign in to reply to this message.
On Mon, Jan 6, 2014 at 10:38 PM, Russ Cox <rsc@golang.org> wrote: > On Fri, Dec 20, 2013 at 1:18 AM, <dvyukov@google.com> wrote: >> >> On 2013/12/19 16:33:40, rsc wrote: >> >>> On Thu, Dec 19, 2013 at 2:29 AM, <mailto:dvyukov@google.com> wrote: >> >> >>> > Wait, can this pool be made global now? >>> > >> >> >>> No. The machine is sized according to the regexp it can execute. >> >> >> I see. >> >> I guess it does not make sense to have a global array of Pools for >> different "size classes". >> >> The remaining concern is: >> Are there cases where a program creates lots of local short-lived >> regexps (one-shot), and it's not possible to replace them with global >> ones? For this case we've introduced a global contended mutex. > > > Yes, that happens in any case where a program computes the regexp at run > time. That's a bit unfortunate. Maybe this should be rolled back. > > Brad, was there any performance improvement here? We can have best of both worlds -- one element local pool + sync.Pool. If we ever put second element into the local pool (which never happens for local regexp's), then switch to sync.Pool.
Sign in to reply to this message.
On Mon, Jan 6, 2014 at 10:38 AM, Russ Cox <rsc@golang.org> wrote: > On Fri, Dec 20, 2013 at 1:18 AM, <dvyukov@google.com> wrote: > >> On 2013/12/19 16:33:40, rsc wrote: >> >> On Thu, Dec 19, 2013 at 2:29 AM, <mailto:dvyukov@google.com> wrote: >>> >> >> > Wait, can this pool be made global now? >>> > >>> >> >> No. The machine is sized according to the regexp it can execute. >>> >> >> I see. >> >> I guess it does not make sense to have a global array of Pools for >> different "size classes". >> >> The remaining concern is: >> Are there cases where a program creates lots of local short-lived >> regexps (one-shot), and it's not possible to replace them with global >> ones? For this case we've introduced a global contended mutex. >> > > Yes, that happens in any case where a program computes the regexp at run > time. That's a bit unfortunate. Maybe this should be rolled back. > > Brad, was there any performance improvement here? > I didn't measure. I was going for code cleanliness and finding places where it should in theory be best, assuming a good implementation of sync.Pool.
Sign in to reply to this message.
Let's just roll back this CL and end this discussion until there is an important performance case to analyze. Thanks. Russ
Sign in to reply to this message.
sure On Mon, Jan 6, 2014 at 12:34 PM, Russ Cox <rsc@golang.org> wrote: > Let's just roll back this CL and end this discussion until there is an > important performance case to analyze. > > Thanks. > Russ > >
Sign in to reply to this message.
|
