Skip to content
This repository has been archived by the owner on Jun 30, 2023. It is now read-only.

Commit

Permalink
jar: prefer io.ReadFull over io.ReadAll
Browse files Browse the repository at this point in the history
This io.ReadAll() call is responsible for a bit more than half the
allocations observed in a very highly scaled up instance of this [1].

io.ReadAll does repeated allocations [1], starting with a 512 byte
buffer. We know the size of the file, so allocate it all at once.

A future commit should look at a further optimization: using a buffer
pool. That wasn't done in this CL because it requires dealing with the
recursive call.

[1]:

```
(pprof) list checkJAR
Total: 213.06GB
         ...
         .     9.89MB    334:           f, err := zf.Open()
         .          .    335:           if err != nil {
         .          .    336:                   return fmt.Errorf("open file %s: %v", p, err)
         .          .    337:           }
         .          .    338:           defer f.Close()
         .   169.96GB    339:           data, err := io.ReadAll(f)
         .          .    340:           if err != nil {
         .          .    341:                   return fmt.Errorf("read file %s: %v", p, err)
         .          .    342:           }
         .          .    343:           br := bytes.NewReader(data)
         .     3.40GB    344:           r2, err := zip.NewReader(br, br.Size())
         ...

(pprof) list ReadAll
Total: 213.06GB
ROUTINE ======================== io.ReadAll in third_party/go/gc/src/io/io.go
  113.40GB   169.96GB (flat, cum) 79.77% of Total
         .          .    638:func ReadAll(r Reader) ([]byte, error) {
         .          .    639:   b := make([]byte, 0, 512)
         .          .    640:   for {
         .          .    641:           if len(b) == cap(b) {
         .          .    642:                   // Add more capacity (let append pick how much).
  113.40GB   113.40GB    643:                   b = append(b, 0)[:len(b)]
         .          .    644:           }
         .    56.56GB    645:           n, err := r.Read(b[len(b):cap(b)])
         .          .    646:           b = b[:len(b)+n]
         .          .    647:           if err != nil {
         .          .    648:                   if err == EOF {
         .          .    649:                           err = nil
         .          .    650:                   }
```

[1]: https://cs.opensource.google/go/go/+/master:src/io/io.go;l=638;drc=2580d0e08d5e9f979b943758d3c49877fb2324cb
  • Loading branch information
aktau committed Feb 28, 2022
1 parent bf524fa commit 4b23cd3
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions jar/jar.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,12 +385,12 @@ func (c *checker) checkFile(zf *zip.File, depth int, size int64, jar string) err
if err != nil {
return fmt.Errorf("open file %s: %v", p, err)
}
data, err := io.ReadAll(f)
buf, err := readFull(f, fi)
f.Close() // Recycle the flate buffer earlier, we're going to recurse.
if err != nil {
return fmt.Errorf("read file %s: %v", p, err)
}
br := bytes.NewReader(data)
br := bytes.NewReader(buf)
r2, err := zip.NewReader(br, br.Size())
if err != nil {
if err == zip.ErrFormat {
Expand All @@ -405,6 +405,18 @@ func (c *checker) checkFile(zf *zip.File, depth int, size int64, jar string) err
return nil
}

func readFull(r io.Reader, fi os.FileInfo) ([]byte, error) {
if !fi.Mode().IsRegular() {
return io.ReadAll(r) // If not a regular file, size may not be accurate.
}
buf := make([]byte, fi.Size())
n, err := io.ReadFull(r, buf)
if err != nil || n != len(buf) {
return nil, err
}
return buf, nil
}

// needsJndiManagerCheck returns true if there's something that we could learn by checking
// JndiManager bytecode with checkJndiManager.
func (c *checker) needsJndiManagerCheck() bool {
Expand Down

0 comments on commit 4b23cd3

Please sign in to comment.