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.

Sign up for GitHub

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

Jump to bottom

Optimize document transform #1847

Merged
merged 1 commit into from Oct 26, 2021
Merged

Optimize document transform #1847

merged 1 commit into from Oct 26, 2021

Conversation

MarinPostma
Copy link
Contributor

integrate the optimization from meilisearch/milli#402.

optimize payload read, by reading it to RAM first instead of streaming it. This means that the payload must fit into RAM, which should not be a problem.

Add BufWriter to the obkv writer to improve write speed.

I have measured a gain of 40-45% in speed after these optimizations.

@MarinPostma
optimize document transform
4ac005b 
fix error types

bump milli
irevoire
irevoire approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

This looks good to me. Why are we deleting a bunch of tests though?

@irevoire irevoire added this to the v0.24.0 milestone Oct 26, 2021
@irevoire irevoire marked this pull request as ready for review October 26, 2021 13:58
@curquiza curquiza changed the title optimize document transform Optimize document transform Oct 26, 2021
@curquiza
Copy link
Member

curquiza commented Oct 26, 2021 •
edited

@MarinPostma @irevoire can you confirm this PR solves

?

@MarinPostma
Copy link
Contributor Author

@irevoire they moved to milli instead

@curquiza since we bumped then yes for 1, but for two let's leave it open a bit more to see if it is sufficient? You decide.

@irevoire
Copy link
Member

I'm ok to merge then 👍
Great work! 🎉

@MarinPostma MarinPostma mentioned this pull request Oct 26, 2021
Closed
@curquiza
Copy link
Member

I'm ok for the merge then! :)

@curquiza curquiza linked an issue Oct 26, 2021 that may be closed by this pull request
Increase limit: number of positions (~ words) per attribute #1770
Closed
3 tasks
@MarinPostma
Copy link
Contributor Author

bors merge

1 similar comment
@irevoire
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 26, 2021

Already running a review

@bors
Copy link
Contributor

bors bot commented Oct 26, 2021

Build succeeded:

@bors bors bot merged commit 0a9d6e8 into main Oct 26, 2021
@bors bors bot deleted the optimize-document-transform branch October 26, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irevoire irevoire irevoire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.24.0
Development

Successfully merging this pull request may close these issues.

Increase limit: number of positions (~ words) per attribute
3 participants
@MarinPostma @curquiza @irevoire