Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pre-allocate decoding errors #690

Merged
merged 2 commits into from
Jul 5, 2016

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Jul 1, 2016

I saw that the dynamic creation of errors was contributing a lot to the heap profile (see below)
I haven't tested extensively yet, but this should fix it

(pprof) top30 -cum
62951786 of 63347642 total (99.38%)
Dropped 41 nodes (cum <= 316738)
      flat  flat%   sum%        cum   cum%
  57502851 90.77% 90.77%   63347642   100%  [metric_tank]
         0     0% 90.77%   35258445 55.66%  github.com/Shopify/sarama.(*realDecoder).getInt64Array
   3922373  6.19% 96.97%    3922373  6.19%  regexp/syntax.(*compiler).quest
    680349  1.07% 98.04%     680349  1.07%  crypto/x509/pkix.(*Name).FillFromRDNSequence
    444790   0.7% 98.74%     504731   0.8%  crypto/x509/pkix.Name.ToRDNSequence
         0     0% 98.74%     491660  0.78%  github.com/Shopify/sarama.(*OffsetFetchRequest).decode
    401423  0.63% 99.38%     401423  0.63%  regexp/syntax.(*parser).factor
(pprof) list getInt64Array
Total: 63347642
ROUTINE ======================== github.com/Shopify/sarama.(*realDecoder).getInt64Array in /home/dieter/go/src/github.com/Shopify/sarama/real_decoder.go
         0   35258445 (flat, cum) 55.66% of Total
         .          .    153:}
         .          .    154:
         .          .    155:func (rd *realDecoder) getInt64Array() ([]int64, error) {
         .          .    156:   if rd.remaining() < 4 {
         .          .    157:       rd.off = len(rd.raw)
         .      10923    158:       return nil, ErrInsufficientData
         .          .    159:   }
         .          .    160:   n := int(binary.BigEndian.Uint32(rd.raw[rd.off:]))
         .          .    161:   rd.off += 4
         .          .    162:
         .          .    163:   if rd.remaining() < 8*n {
         .    1773176    164:       rd.off = len(rd.raw)
         .          .    165:       return nil, ErrInsufficientData
         .          .    166:   }
         .          .    167:
         .          .    168:   if n == 0 {
         .    1180404    169:       return nil, nil
         .          .    170:   }
         .          .    171:
         .          .    172:   if n < 0 {
         .   32293942    173:       return nil, PacketDecodingError{"invalid array length"}
         .          .    174:   }
         .          .    175:
         .          .    176:   ret := make([]int64, n)
         .          .    177:   for i := range ret {
         .          .    178:       ret[i] = int64(binary.BigEndian.Uint64(rd.raw[rd.off:]))
(pprof) 

@eapache
Copy link
Contributor

eapache commented Jul 1, 2016

What kind of environment are you running that you're getting a substantial number of errors? I've never bothered optimising these paths because in practice kafka brokers always send well-formed messages (over TCP with a checksum too) so none of these errors are ever hit.

var ErrInvalidArrayLength = PacketDecodingError{"invalid array length"}
var ErrInvalidByteSliceLength = PacketDecodingError{"invalid byteslice length"}
var ErrInvalidStringLength = PacketDecodingError{"invalid string length"}
var ErrInvalidSubsetSize = PacketDecodingError{"invalid subset size"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make more sense to make these unexported and move them to the top of real_decoder.go, I can't think of a case where the caller would need to be able to distinguish between them.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jul 1, 2016

This is in a development environment, and my first foray into Kafka (though with substantial traffic/load). I'll move the errors to real_decoder and privatise them. While the issue may be rare it should be a non-issue IMHO. I wouldn't want a flood of protocol errors to be exacerbated by excessive GC in prod.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jul 4, 2016

@eapache how does it look now?

@eapache
Copy link
Contributor

eapache commented Jul 5, 2016

Great, thanks!

@eapache eapache merged commit 1a3d124 into IBM:master Jul 5, 2016
@wvanbergen
Copy link
Contributor

Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants