From 0d03621d45a3dd9a6f2600596a81d50f8d514392 Mon Sep 17 00:00:00 2001 From: Keegan Carruthers-Smith Date: Wed, 29 Nov 2023 14:21:38 +0100 Subject: [PATCH] revert use of mmap-go (#706) We have a suspicion that when we switched to using the mmap-go library it contributed to an issue where zoekt-webserver stops responding on some linux versions. Our suspicion has something to do from us hardcoding the page size to 4k to asking the kernel and how that interacts with THP. Given we don't actually build for windows anymore, we are partially reverting the mmap changes from 7424ac84bab3ec9ae247339909ffd84e5e3b1338 Test Plan: go test ./... on darwin. Then CI for linux testing. --- go.mod | 1 - go.sum | 2 - indexfile_other.go | 62 +++++++++++++++++++++++++++++++ indexfile.go => indexfile_unix.go | 35 +++++------------ 4 files changed, 72 insertions(+), 28 deletions(-) create mode 100644 indexfile_other.go rename indexfile.go => indexfile_unix.go (63%) diff --git a/go.mod b/go.mod index a91430df5..fbdf0ac5f 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/andygrunwald/go-gerrit v0.0.0-20230628115649-c44fe2fbf2ca github.com/bmatcuk/doublestar v1.3.4 github.com/dustin/go-humanize v1.0.1 - github.com/edsrzf/mmap-go v1.1.0 github.com/felixge/fgprof v0.9.3 github.com/fsnotify/fsnotify v1.6.0 github.com/gfleury/go-bitbucket-v1 v0.0.0-20230626192437-8d7be5866751 diff --git a/go.sum b/go.sum index b85a4d272..e12f6305e 100644 --- a/go.sum +++ b/go.sum @@ -78,8 +78,6 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= -github.com/edsrzf/mmap-go v1.1.0 h1:6EUwBLQ/Mcr1EYLE4Tn1VdW1A4ckqCQWZBw8Hr0kjpQ= -github.com/edsrzf/mmap-go v1.1.0/go.mod h1:19H/e8pUPLicwkyNgOykDXkJ9F0MHE+Z52B8EIth78Q= github.com/elazarl/goproxy v0.0.0-20221015165544-a0805db90819 h1:RIB4cRk+lBqKK3Oy0r2gRX4ui7tuhiZq2SuTtTCi0/0= github.com/elazarl/goproxy v0.0.0-20221015165544-a0805db90819/go.mod h1:Ro8st/ElPeALwNFlcTpWmkr6IoMFfkjXAvTHpevnDsM= github.com/emirpasic/gods v1.18.1 h1:FXtiHYKDGKCW2KzwZKx0iC0PQmdlorYgdFG9jPXJ1Bc= diff --git a/indexfile_other.go b/indexfile_other.go new file mode 100644 index 000000000..f192d9beb --- /dev/null +++ b/indexfile_other.go @@ -0,0 +1,62 @@ +// Copyright 2016 Google Inc. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build !linux && !darwin +// +build !linux,!darwin + +package zoekt + +import ( + "fmt" + "os" +) + +// NewIndexFile returns a new index file. The index file takes +// ownership of the passed in file, and may close it. +func NewIndexFile(f *os.File) (IndexFile, error) { + return &indexFileFromOS{f}, nil +} + +type indexFileFromOS struct { + f *os.File +} + +func (f *indexFileFromOS) Read(off, sz uint32) ([]byte, error) { + r := make([]byte, sz) + _, err := f.f.ReadAt(r, int64(off)) + return r, err +} + +func (f indexFileFromOS) Size() (uint32, error) { + fi, err := f.f.Stat() + if err != nil { + return 0, err + } + + sz := fi.Size() + + if sz >= maxUInt32 { + return 0, fmt.Errorf("overflow") + } + + return uint32(sz), nil +} + +func (f indexFileFromOS) Close() { + f.f.Close() +} + +func (f indexFileFromOS) Name() string { + return f.f.Name() +} diff --git a/indexfile.go b/indexfile_unix.go similarity index 63% rename from indexfile.go rename to indexfile_unix.go index 2cb7941fd..fea4fae76 100644 --- a/indexfile.go +++ b/indexfile_unix.go @@ -12,24 +12,22 @@ // See the License for the specific language governing permissions and // limitations under the License. +//go:build linux || darwin + package zoekt import ( "fmt" "log" "os" - "runtime" - // cross-platform memory-mapped file package. - // Benchmarks the same speed as syscall/unix Mmap - // see https://github.com/peterguy/benchmark-mmap - mmap "github.com/edsrzf/mmap-go" + "golang.org/x/sys/unix" ) type mmapedIndexFile struct { name string size uint32 - data mmap.MMap + data []byte } func (f *mmapedIndexFile) Read(off, sz uint32) ([]byte, error) { @@ -48,25 +46,11 @@ func (f *mmapedIndexFile) Size() (uint32, error) { } func (f *mmapedIndexFile) Close() { - if err := f.data.Unmap(); err != nil { - log.Printf("WARN failed to memory unmap %s: %v", f.name, err) + if err := unix.Munmap(f.data); err != nil { + log.Printf("WARN failed to Munmap %s: %v", f.name, err) } } -func bufferSize(f *mmapedIndexFile) int { - // On Unix/Linux, mmap likes to allocate memory in - // page-sized chunks, so round up to the OS page size. - // mmap will zero-fill the extra bytes. - // On Windows, the Windows API CreateFileMapping method - // requires a buffer the same size as the file. - bsize := int(f.size) - if runtime.GOOS != "windows" { - pagesize := os.Getpagesize() - 1 - bsize = (bsize + pagesize) &^ pagesize - } - return bsize -} - // NewIndexFile returns a new index file. The index file takes // ownership of the passed in file, and may close it. func NewIndexFile(f *os.File) (IndexFile, error) { @@ -86,10 +70,11 @@ func NewIndexFile(f *os.File) (IndexFile, error) { size: uint32(sz), } - r.data, err = mmap.MapRegion(f, bufferSize(r), mmap.RDONLY, 0, 0) + rounded := (r.size + 4095) &^ 4095 + r.data, err = unix.Mmap(int(f.Fd()), 0, int(rounded), unix.PROT_READ, unix.MAP_SHARED) if err != nil { - return nil, fmt.Errorf("NewIndexFile: unable to memory map %s: %w", f.Name(), err) + return nil, err } - return r, nil + return r, err }