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

avoid prepare_doc allocation #1610

Merged
merged 1 commit into from
Oct 11, 2022
Merged

avoid prepare_doc allocation #1610

merged 1 commit into from
Oct 11, 2022

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Oct 9, 2022

avoid prepare_doc allocation, ~10% more thoughput best case

avoid prepare_doc allocation, ~10% more thoughput best case
@codecov-commenter
Copy link

Codecov Report

Merging #1610 (19bc196) into main (5f565e7) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1610      +/-   ##
==========================================
- Coverage   93.78%   93.77%   -0.02%     
==========================================
  Files         252      252              
  Lines       46380    46389       +9     
==========================================
+ Hits        43499    43502       +3     
- Misses       2881     2887       +6     
Impacted Files Coverage Δ
src/indexer/segment_writer.rs 96.34% <100.00%> (-0.07%) ⬇️
src/schema/document.rs 92.00% <100.00%> (-0.82%) ⬇️
src/store/mod.rs 99.23% <100.00%> (ø)
src/store/writer.rs 100.00% <100.00%> (ø)
src/fastfield/multivalued/mod.rs 98.45% <0.00%> (-0.78%) ⬇️
src/postings/stacker/expull.rs 98.57% <0.00%> (-0.48%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@adamreichold
Copy link
Collaborator

This is nice! Did you find this via allocation profiling? If so, which tool did you use?

@PSeitz
Copy link
Contributor Author

PSeitz commented Oct 9, 2022

This is nice! Did you find this via allocation profiling? If so, which tool did you use?

@adamreichold I used perf perf record --call-graph dwarf. There was a relative large chunk of free in the calling function, so I was checking for allocations (free is still relative large though). It's also visible with an allocation profiler valgrind --tool=dhat.

@@ -191,6 +191,34 @@ impl Document {
pub fn get_first(&self, field: Field) -> Option<&Value> {
self.get_all(field).next()
}

/// Serializes stored field values.
pub fn serialize_stored<W: Write>(&self, schema: &Schema, writer: &mut W) -> io::Result<()> {
Copy link
Collaborator

@fulmicoton fulmicoton Oct 10, 2022

Choose a reason for hiding this comment

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

can we make this a function in store/writer.rs?

Copy link
Contributor Author

@PSeitz PSeitz Oct 10, 2022

Choose a reason for hiding this comment

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

I think the abstraction is better like this, so the store doesn't need to know about Document internals like FieldValues and de/serialization is in the same file

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes you are right!

Copy link
Collaborator

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

Nice!

I have one blocking comment.

@fulmicoton
Copy link
Collaborator

@PSeitz Awesome. I love everything that removes allocation without makign the code too complex!
If you have creative ideas on how to remove the Doc related allocation that would be nice too.

I think we still have some in the tokenizer (T_T) and the document forces some too.
I think we can remove all of them.

@fulmicoton fulmicoton merged commit 8b69aab into main Oct 11, 2022
@fulmicoton fulmicoton deleted the remove_prepare_doc branch October 11, 2022 05:15
This was referenced Jan 13, 2023
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.

4 participants