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

Add lightweight keyset wrapper #527

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bsless
Copy link
Contributor

@bsless bsless commented Aug 24, 2021

Saves on allocation and provides an immutable facade to underlying
regular set

Followup to #522
Part of #513

Saves on allocation and provides an immutable facade to underlying
regular set
@bsless bsless force-pushed the lightweigh-keyset-wrapper branch from 98bc16b to bed82e1 Compare August 24, 2021 16:17
@bsless
Copy link
Contributor Author

bsless commented Aug 24, 2021

I believe the difference is significant enough to consider this given the benefit for larger schemas:

(cc/quick-bench
 ;; 1.799482 µs -> 1.694608 µs
 (m/-parse-entries [[:x int?] [:y {:optional true} boolean?] [:z string?]] nil nil))


(cc/quick-bench
 ;; 5.412949 µs -> 4.899880 µs
 (m/-parse-entries [[:a int?]
                    [:b {:optional true} boolean?]
                    [:c string?]
                    [:u int?]
                    [:v int?]
                    [:w int?]
                    [:x int?]
                    [:y int?]
                    [:z int?]]
                   nil nil))

@bsless
Copy link
Contributor Author

bsless commented Sep 12, 2021

@ikitommi any interest in this patch? I can rebase it.

@ikitommi
Copy link
Member

Thanks for your work on this. Could you check the #539 , in which, I swapped the set to be a map with key->index mapping. This allows effective patching with reusing the parse-results.

@ikitommi
Copy link
Member

... the map generation could be improved I think, just collect the key-index pairs, create a arraymap in the end?

@bsless
Copy link
Contributor Author

bsless commented Sep 12, 2021

The same mechanism I used for the set could be used here - use a mutable java collection and expose it later by an immutable "view"

@ikitommi
Copy link
Member

maybe just collecting the key + values into object-array and create persistent with PersistentArrayMap/createWithCheck? looks fast, haven't tested.

@bsless
Copy link
Contributor Author

bsless commented Sep 12, 2021

Creating an ArrayMap is tricky. You don't want to do it for over 8 keys, and createWithCheck will be worst-case quadratic for it.

@ikitommi
Copy link
Member

ikitommi commented Sep 12, 2021

we already do object-arrays for everything else, just one with double size? looks fast and does fast duplicate key check:

(let [m {:a 1, :b 2, :c 3, :d 4, :e 5}
      a (object-array (* 2 (count m)))]
  (doseq [[i [k v]] (map-indexed vector m)]
    (aset a (* i 2) k)
    (aset a (inc (* i 2)) v))
  (assert (= m (clojure.lang.PersistentArrayMap/createWithCheck a)))
  ;; 27ns
  (cc/quick-bench
   (clojure.lang.PersistentArrayMap/createWithCheck a)))

@bsless
Copy link
Contributor Author

bsless commented Sep 12, 2021

static public PersistentArrayMap createWithCheck(Object[] init){
	for(int i=0;i< init.length;i += 2)
		{
		for(int j=i+2;j<init.length;j += 2)
			{
			if(equalKey(init[i],init[j]))
				throw new IllegalArgumentException("Duplicate key: " + init[i]);
			}
		}
	return new PersistentArrayMap(init);
}

It's not slow, but need to be aware it's quadratic, don't know what which size it becomes too heavy

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.

2 participants