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

Fix IO Stats computation #119

Merged
merged 4 commits into from
Jul 22, 2022

Conversation

Thejas-bhat
Copy link
Contributor

The newly introduced stats in the PR #1702 and #117
are currently computed all time the irrespective of whether or not its needed. Since it is in a fairly hot code path, it could affect the performance when these stats are not needed. Hence, this PR aims to hide those stats computation behind a flag which controls the same.

@Thejas-bhat Thejas-bhat changed the title Keeping the IO stats computation behind a flag Making IO Stats computation optional Jul 15, 2022
@Thejas-bhat Thejas-bhat force-pushed the statsUnderFlag branch 2 times, most recently from 53f09d7 to 3cb7cf7 Compare July 20, 2022 14:53
Copy link
Member

@abhinavdangeti abhinavdangeti left a comment

Choose a reason for hiding this comment

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

Looking ok I think.

intDecoder.go Outdated
@@ -59,7 +59,9 @@ func newChunkedIntDecoder(buf []byte, offset uint64, rv *chunkedIntDecoder) *chu
rv.chunkOffsets[i], read = binary.Uvarint(buf[offset+n : offset+n+binary.MaxVarintLen64])
n += uint64(read)
}

Copy link
Member

Choose a reason for hiding this comment

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

Undo the changes in this file, nothing related here.

@abhinavdangeti
Copy link
Member

And rebase over commit 7204400.

@abhinavdangeti abhinavdangeti changed the title Making IO Stats computation optional Fix IO Stats computation Jul 22, 2022
@abhinavdangeti abhinavdangeti marked this pull request as ready for review July 22, 2022 17:11
@abhinavdangeti
Copy link
Member

Rebased @Thejas-bhat 's changes over tip of master.

@abhinavdangeti abhinavdangeti merged commit 5dd118e into blevesearch:master Jul 22, 2022
abhinavdangeti pushed a commit that referenced this pull request Nov 30, 2022
An unaligned atomic bug was unfortunately introduced in #119 because the
`bytesWritten` field was placed at the end of the `chunkedContentCoder`
struct.

This places this after the bytes.Buffers and the bool causing it to be
misaligned.

The ideal placement of this variable is not entirely clear but placing
it before the progresiveWrite bool should help.

An alternative would be to just place this atomic field at the top of
the struct then there would be no risk of it becoming misaligned in
future.

I moved a few things around to reduce the size of the struct too but it
could be possible to adjust things a little more to make the struct a
little smaller.

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/zapx that referenced this pull request Dec 5, 2022
An unaligned atomic bug was unfortunately introduced in blevesearch#119 because the
`bytesWritten` field was placed at the end of the `chunkedContentCoder`
struct.

This places this after the bytes.Buffers and the bool causing it to be
misaligned.

The ideal placement of this variable is not entirely clear but placing
it before the progresiveWrite bool should help.

An alternative would be to just place this atomic field at the top of
the struct then there would be no risk of it becoming misaligned in
future.

I moved a few things around to reduce the size of the struct too but it
could be possible to adjust things a little more to make the struct a
little smaller.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants