Optimize OStream implementation #1399
Conversation
richardartoul
changed the title
richardartoul
requested review from
robskillington,
martin-mao,
xichen2020,
simonrobb and
justinjc
and removed request for
xichen2020
Codecov Report
@@ Coverage Diff @@
## master #1399 +/- ##
======================================
Coverage 63.6% 63.6%
======================================
Files 753 753
Lines 64733 64733
======================================
Hits 41191 41191
Misses 20455 20455
Partials 3087 3087
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1399 +/- ##
========================================
- Coverage 70.8% 70.8% -0.1%
========================================
Files 832 832
Lines 71370 71400 +30
========================================
- Hits 50559 50553 -6
- Misses 17511 17535 +24
- Partials 3300 3312 +12
Continue to review full report at Codecov.
|
| var ( | ||
| currCap = cap(os.rawBuffer) | ||
| currLen = len(os.rawBuffer) | ||
| availableCap = currCap - currLen |
There was a problem hiding this comment.
Nit: Given that availableCap is only used in the next line, may as well just do the calc of missingCap in 1 line?
src/dbnode/encoding/ostream.go
Outdated
| return os.checked, os.pos | ||
| } | ||
|
|
||
| // repairCheckedBytes makes suer that the checked.Bytes wraps the rawBuffer as |
| @@ -180,5 +234,21 @@ func (os *ostream) Reset(buffer checked.Bytes) { | |||
|
|
|||
| // Rawbytes returns the Osteam's raw bytes | |||
| func (os *ostream) Rawbytes() (checked.Bytes, int) { | |||
| return os.rawBuffer, os.pos | |||
| os.repairCheckedBytes() | |||
| return os.checked, os.pos | |||
There was a problem hiding this comment.
This changes the cost accounting accuracy right? eg, checked now will only give the cost when this was called and not the underlying bytes which may be written to afterwards. Not sure if it matters, how long do we hold the ref from this call for and what is the cost accounting used for?
There was a problem hiding this comment.
As far as I'm aware we don't use checked bytes for tracking cost, we use them for tracking ownership.
This changes the semantics slightly, but the way .the OStream is used this should be fine. Basically what I'm doing is while the OStream is holding onto the checked bytes, it uses the raw buffer to write to them, and only when it exposes the bytes to someone else does it use the checked bytes.
The only thing that could cause issues is if someone got a hold of the checked bytes from this thing, and then they kept writing to the ostream and the rawbuffer and checked bytes they grabbed earlier got out of sync, but thats not how we use this structure in the encoder luckily
richardartoul
force-pushed
the
ra/optimize-ostream
branch
from
d398624 to
261eaab
Compare
When writing annotation heavy code, we spend 80% of our time in this routine:
This P.R makes that code-path 10x faster
Before
After