[pbs-devel] [PATCH proxmox 1/2] api-macro: fix broken binary ident search

[pbs-devel] [PATCH proxmox 1/2] api-macro: fix broken binary ident search

[Dominik Csapak]

the 'properties_' list is sorted by the the literal string of a
fieldname, but we binary-search for the 'ident_str' (which may be
different, since we map '-' to '_' for example)

by creating a hashmap to map from ident to index, we can do a simple
lookup in that case that will work

benchmarks showed no measurable performance difference

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-api-macro/src/api/mod.rs | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/proxmox-api-macro/src/api/mod.rs b/proxmox-api-macro/src/api/mod.rs
index 2d2dada..0071e81 100644
--- a/proxmox-api-macro/src/api/mod.rs
+++ b/proxmox-api-macro/src/api/mod.rs
@@ -8,6 +8,7 @@
 //! The handling of methods vs type definitions happens in their corresponding submodules.
 
 use std::convert::{TryFrom, TryInto};
+use std::collections::HashMap;
 
 use anyhow::Error;
 
@@ -378,12 +379,14 @@ impl SchemaItem {
 /// Contains a sorted list of properties:
 pub struct SchemaObject {
     properties_: Vec<(FieldName, bool, Schema)>,
+    ident_hash: HashMap<String, usize>,
 }
 
 impl SchemaObject {
     pub fn new() -> Self {
         Self {
             properties_: Vec::new(),
+            ident_hash: HashMap::new(),
         }
     }
 
@@ -394,6 +397,9 @@ impl SchemaObject {
 
     fn sort_properties(&mut self) {
         self.properties_.sort_by(|a, b| (a.0).cmp(&b.0));
+        for (idx, prop) in self.properties_.iter().enumerate() {
+            self.ident_hash.insert(prop.0.as_ident_str().to_string(), idx);
+        }
     }
 
     fn try_extract_from(obj: &mut JSONObject) -> Result<Self, syn::Error> {
@@ -422,6 +428,7 @@ impl SchemaObject {
                         Ok(properties)
                     },
                 )?,
+            ident_hash: HashMap::new(),
         };
         this.sort_properties();
         Ok(this)
@@ -439,22 +446,16 @@ impl SchemaObject {
     }
 
     fn find_property_by_ident(&self, key: &str) -> Option<&(FieldName, bool, Schema)> {
-        match self
-            .properties_
-            .binary_search_by(|p| p.0.as_ident_str().cmp(key))
-        {
-            Ok(idx) => Some(&self.properties_[idx]),
-            Err(_) => None,
+        match self.ident_hash.get(key) {
+            Some(idx) => Some(&self.properties_[*idx]),
+            None => None,
         }
     }
 
     fn find_property_by_ident_mut(&mut self, key: &str) -> Option<&mut (FieldName, bool, Schema)> {
-        match self
-            .properties_
-            .binary_search_by(|p| p.0.as_ident_str().cmp(key))
-        {
-            Ok(idx) => Some(&mut self.properties_[idx]),
-            Err(_) => None,
+        match self.ident_hash.get(key) {
+            Some(idx) => Some(&mut self.properties_[*idx]),
+            None => None,
         }
     }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox 2/2] api-macro: relax Fieldname rules

[Dominik Csapak]

by replacing more characters ('.','+') by '_' and prefix them when
it starts with a number

we sometimes need to parse such fields, e.g in serde attributes like
 #[serde(rename = "802.3ad")]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-api-macro/src/util.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/proxmox-api-macro/src/util.rs b/proxmox-api-macro/src/util.rs
index 6ac39c2..9748131 100644
--- a/proxmox-api-macro/src/util.rs
+++ b/proxmox-api-macro/src/util.rs
@@ -29,7 +29,11 @@ pub struct FieldName {
 
 impl FieldName {
     pub fn new(name: String, span: Span) -> Self {
-        let ident_str = name.replace("-", "_");
+        let mut ident_str = name.replace(['-', '.', '+'].as_ref(), "_");
+
+        if ident_str.chars().next().unwrap().is_numeric() {
+            ident_str.insert(0, '_');
+        }
 
         Self {
             ident: Ident::new(&ident_str, span),
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox 1/2] api-macro: fix broken binary ident search

[Dietmar Maurer]

> @@ -394,6 +397,9 @@ impl SchemaObject {
>  
>      fn sort_properties(&mut self) {
>          self.properties_.sort_by(|a, b| (a.0).cmp(&b.0));
> +        for (idx, prop) in self.properties_.iter().enumerate() {
> +            self.ident_hash.insert(prop.0.as_ident_str().to_string(), idx);
> +        }
>      }
>  

Above looks really wrong to me. Why should we repopulate the whole hash in sort_properties()?

Re: [pbs-devel] [PATCH proxmox 1/2] api-macro: fix broken binary ident search

[Dietmar Maurer]

> On 09/17/2020 6:25 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > @@ -394,6 +397,9 @@ impl SchemaObject {
> >  
> >      fn sort_properties(&mut self) {
> >          self.properties_.sort_by(|a, b| (a.0).cmp(&b.0));
> > +        for (idx, prop) in self.properties_.iter().enumerate() {
> > +            self.ident_hash.insert(prop.0.as_ident_str().to_string(), idx);
> > +        }
> >      }
> >  
> 
> Above looks really wrong to me. Why should we repopulate the whole hash in sort_properties()?

Ok, finally got it. We only call sort once there, so this is actually ok.

[pbs-devel] applied: [PATCH proxmox 1/2] api-macro: fix broken binary ident search

[Dietmar Maurer]

applied both patches