From d19c8672bb66eaf6405ca726f6b6eaa0021ea61c Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Wed, 23 Nov 2022 15:50:53 -0500 Subject: [PATCH 1/7] perf: limit reindex to when exact_attributes changes --- milli/src/index.rs | 5 ++--- milli/src/update/settings.rs | 23 ++++++++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/milli/src/index.rs b/milli/src/index.rs index 5910a305c..d9636634d 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -1145,9 +1145,8 @@ impl Index { } /// Clears the exact attributes from the store. - pub(crate) fn delete_exact_attributes(&self, txn: &mut RwTxn) -> Result<()> { - self.main.delete::<_, Str>(txn, main_key::EXACT_ATTRIBUTES)?; - Ok(()) + pub(crate) fn delete_exact_attributes(&self, txn: &mut RwTxn) -> Result { + Ok(self.main.delete::<_, Str>(txn, main_key::EXACT_ATTRIBUTES)?) } pub fn max_values_per_facet(&self, txn: &RoTxn) -> heed::Result> { diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index 6da32d73f..8220ed3ab 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -465,14 +465,23 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_exact_attributes(&mut self) -> Result { match self.exact_attributes { Setting::Set(ref attrs) => { - let attrs = attrs.iter().map(String::as_str).collect::>(); - self.index.put_exact_attributes(self.wtxn, &attrs)?; - Ok(true) - } - Setting::Reset => { - self.index.delete_exact_attributes(self.wtxn)?; - Ok(true) + let old_attrs = self + .index + .exact_attributes(self.wtxn)? + .iter() + .cloned() + .map(String::from) + .collect::>(); + + if attrs != &old_attrs { + let attrs = attrs.iter().map(String::as_str).collect::>(); + self.index.put_exact_attributes(self.wtxn, &attrs)?; + Ok(true) + } else { + Ok(false) + } } + Setting::Reset => Ok(self.index.delete_exact_attributes(self.wtxn)?), Setting::NotSet => Ok(false), } } From bb9e33bf85f5fd69d49b72cd6fc43b97f951d4ff Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Wed, 23 Nov 2022 22:01:46 -0500 Subject: [PATCH 2/7] perf: Prevent reindex in searchable reset case when not needed --- milli/src/update/settings.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index 8220ed3ab..586198c52 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -373,13 +373,11 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { &new_fields_ids_map, )?; self.index.put_fields_ids_map(self.wtxn, &new_fields_ids_map)?; + Ok(true) } - Setting::Reset => { - self.index.delete_all_searchable_fields(self.wtxn)?; - } + Setting::Reset => Ok(self.index.delete_all_searchable_fields(self.wtxn)?), Setting::NotSet => return Ok(false), } - Ok(true) } fn update_stop_words(&mut self) -> Result { From ed29cceae940d580ccdf5407c7dea0c07d168d86 Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Wed, 23 Nov 2022 22:33:06 -0500 Subject: [PATCH 3/7] perf: Prevent reindex in searchable set case when not needed --- milli/src/update/settings.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index 586198c52..aed2d951e 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -349,6 +349,16 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_searchable(&mut self) -> Result { match self.searchable_fields { Setting::Set(ref fields) => { + let did_change = self + .index + .searchable_fields(self.wtxn)? + .map(|f| f.into_iter().map(String::from).collect::>()) + .map(|old_fields| fields != &old_fields) + .unwrap_or(true); // if old_fields was None before, it was changed + if !did_change { + return Ok(false); + } + // every time the searchable attributes are updated, we need to update the // ids for any settings that uses the facets. (distinct_fields, filterable_fields). let old_fields_ids_map = self.index.fields_ids_map(self.wtxn)?; @@ -376,7 +386,7 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { Ok(true) } Setting::Reset => Ok(self.index.delete_all_searchable_fields(self.wtxn)?), - Setting::NotSet => return Ok(false), + Setting::NotSet => Ok(false), } } From 2db738dbac9b3e35b69a989b835e27b58f44d5a1 Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Sat, 26 Nov 2022 13:26:39 -0500 Subject: [PATCH 4/7] refactor: rewrite method chain to be more readable --- milli/src/update/settings.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index aed2d951e..7d281262a 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -349,12 +349,17 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_searchable(&mut self) -> Result { match self.searchable_fields { Setting::Set(ref fields) => { - let did_change = self - .index - .searchable_fields(self.wtxn)? - .map(|f| f.into_iter().map(String::from).collect::>()) - .map(|old_fields| fields != &old_fields) - .unwrap_or(true); // if old_fields was None before, it was changed + // Check to see if the searchable fields changed before doing anything else + let old_fields = self.index.searchable_fields(self.wtxn)?; + let did_change = match old_fields { + // If old_fields is Some, let's check to see if the fields actually changed + Some(old_fields) => { + let new_fields = fields.iter().map(String::as_str).collect::>(); + new_fields != old_fields + } + // If old_fields is None, the fields have changed (because they are being set) + None => true, + }; if !did_change { return Ok(false); } From e0d24104a3c66a9a597288032212f7ae1cc06e9a Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Sat, 26 Nov 2022 13:33:19 -0500 Subject: [PATCH 5/7] refactor: Rewrite another method chain to be more readable --- milli/src/update/settings.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/milli/src/update/settings.rs b/milli/src/update/settings.rs index 7d281262a..fc7e6bc03 100644 --- a/milli/src/update/settings.rs +++ b/milli/src/update/settings.rs @@ -478,13 +478,8 @@ impl<'a, 't, 'u, 'i> Settings<'a, 't, 'u, 'i> { fn update_exact_attributes(&mut self) -> Result { match self.exact_attributes { Setting::Set(ref attrs) => { - let old_attrs = self - .index - .exact_attributes(self.wtxn)? - .iter() - .cloned() - .map(String::from) - .collect::>(); + let old_attrs = self.index.exact_attributes(self.wtxn)?; + let old_attrs = old_attrs.into_iter().map(String::from).collect::>(); if attrs != &old_attrs { let attrs = attrs.iter().map(String::as_str).collect::>(); From d3182f38307dc8be91f604a9f300a8b777ceb7f9 Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Mon, 28 Nov 2022 10:02:03 -0500 Subject: [PATCH 6/7] refactor: Change return type to keep consistency with others --- milli/src/index.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/milli/src/index.rs b/milli/src/index.rs index d9636634d..9e4e56de0 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -1145,8 +1145,8 @@ impl Index { } /// Clears the exact attributes from the store. - pub(crate) fn delete_exact_attributes(&self, txn: &mut RwTxn) -> Result { - Ok(self.main.delete::<_, Str>(txn, main_key::EXACT_ATTRIBUTES)?) + pub(crate) fn delete_exact_attributes(&self, txn: &mut RwTxn) -> heed::Result { + self.main.delete::<_, Str>(txn, main_key::EXACT_ATTRIBUTES) } pub fn max_values_per_facet(&self, txn: &RoTxn) -> heed::Result> { From 87e2bc3beda573a72c87647d614393d524bd5b44 Mon Sep 17 00:00:00 2001 From: Gregory Conrad Date: Mon, 28 Nov 2022 13:12:19 -0500 Subject: [PATCH 7/7] fix(reindex): reindex in a few more cases Cases: whenever searchable_fields OR user_defined_searchable_fields is modified --- milli/src/index.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/milli/src/index.rs b/milli/src/index.rs index 9e4e56de0..33c04789d 100644 --- a/milli/src/index.rs +++ b/milli/src/index.rs @@ -560,8 +560,9 @@ impl Index { } pub(crate) fn delete_all_searchable_fields(&self, wtxn: &mut RwTxn) -> heed::Result { - self.delete_searchable_fields(wtxn)?; - self.delete_user_defined_searchable_fields(wtxn) + let did_delete_searchable = self.delete_searchable_fields(wtxn)?; + let did_delete_user_defined = self.delete_user_defined_searchable_fields(wtxn)?; + Ok(did_delete_searchable || did_delete_user_defined) } /// Writes the searchable fields, when this list is specified, only these are indexed.