Object Keys are always Cow::Owned with cowkeys feature

[jszwec]

Hello,

I noticed that object keys are always Cow::Owned with cowkeys feature on.
I tracked it down to serde being responsible for this as Cow has a specific Deserializer implementation that always uses Owned variant.

We could potentially implement our own wrapper around Cow, that would allow us to have either Owned or Borrowed values when possible.
That would bring further performance gains.

Is there anything that you are aware of that would stop us from doing it? If not, I could create a PR with proposed change.

[PSeitz]

Do you have an example?
It depends if serde calls visit_borrowed_str, visit_str or visit_string if Cow::Borrowed can be used.

[jszwec]

Apply this diff and run cargo test cowkeys

diff --git a/src/de.rs b/src/de.rs
index 3b17a5b..a7f42ee 100644
--- a/src/de.rs
+++ b/src/de.rs
@@ -145,6 +145,22 @@ mod tests {

     use crate::Value;

+    #[test]
+    fn cowkeys() {
+        let json_obj = r#"
+            {
+                "bool": true,
+                "escaped\"": true
+            }
+       "#;
+
+        let val: Value = serde_json::from_str(json_obj).unwrap();
+
+        let obj = val.as_object().unwrap();
+        assert!(matches!(obj.as_vec()[0].0, Cow::Borrowed(_)));
+        assert!(matches!(obj.as_vec()[1].0, Cow::Owned(_)));
+    }
+
     #[test]
     fn deserialize_json_test() {
         let json_obj = r#"

This is because of serde, it always uses Cow::Owned -> serde-rs/serde#1852 (comment)

[PSeitz]

In this example you show that it's not always using Cow::Owned like you said, but only if the data needs to be escaped. That behaves like expected.

If you want to avoid it, you'd need to use simd_json as a deserializer.

[jszwec]

@PSeitz, What are you talking about?

"bool" key should be Cow::Borrowed, but it's Owned, and the test fails

did you even run that test?

[PSeitz]

Whoops I'm sorry, I made an error running the test. It's indeed an issue for object keys, values seem to work fine though.

What would you propose? There's already some complexity by having Cow and &str via the feature flag

[jszwec]

yeah values are fine, the issue seems to be only with the keys like you said.

I put together a wrapper just to see if it's gonna work - something like this: CowStr<'a>(Cow<'a, str>) then I implemented a few traits including AsRef, Deref, and Deserialize and it seems to work without making any other changes.

For as long as it doesn't break backward-compatibility somewhere, I think it could look like this:

#[cfg(feature = "cowkeys"]
pub type KeyStrType<'a> = CowStr<'a>

Unfortunately, it means we would also need CowStr to be public.

If you think it can work, I can polish my code and put it in the PR for you to review

[PSeitz]

Currently Cow is exposed in the API, so it would be a breaking change. Deref should get us pretty far though on API compatibility. I would be definitely interested in the fix. Impact on the benchmarks would also be quite interesting.

[jszwec]

Agreed, the good thing is that Cow seems to be exposed in the API only when you call as_vec() and to_vec() on ObjectAsVec, all other methods including iterators are &str based.

I can implement a POC including benchmarks, and then we can think if breaking change is worth it on these two methods, or how else we can approach it