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 data panic #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hengfeiyang
Copy link
Contributor

@hengfeiyang hengfeiyang commented Jul 4, 2022

This is our version by many users using, for now no error found.

  • Fix data out of range
  • Add cache for decompress

@hengfeiyang hengfeiyang mentioned this pull request Jul 5, 2022
chunkI := uint32(docNum) / defaultDocumentChunkSize
storedFieldDecompressed := s.decompressedStoredFieldChunks[chunkI]
storedFieldDecompressed.m.Lock()
if storedFieldDecompressed.data == nil {
Copy link

Choose a reason for hiding this comment

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

This section tries to reduce the amount of repeated work by doing it inside a locked section, have you considered performing the decompression outside the lock and then lock->check->update.

So we might end up with repeated work between multiple requests but less contention between them.

what do you thing? (and also @mschoch)

Copy link
Contributor Author

@hengfeiyang hengfeiyang Jul 6, 2022

Choose a reason for hiding this comment

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

@voldyman We don't want decompress all chunks, just want decompress the chunk which visited. so we decompress when visit, and to avoid decompress multiple, so we use lock to ensure just do once.

And about you talked i am not sure how to implement it, i think you can try to create a new PR or give some code for that.

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