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

Late declarations of lower-cost conversions are ineffective #61

Open
DerGuteMoritz opened this issue Sep 12, 2022 · 3 comments
Open

Late declarations of lower-cost conversions are ineffective #61

DerGuteMoritz opened this issue Sep 12, 2022 · 3 comments

Comments

@DerGuteMoritz
Copy link
Collaborator

DerGuteMoritz commented Sep 12, 2022

Declaring new conversions via def-conversion which would make conversion between two types less costly are ineffective if the conversion in question has occurred at least once before. This is caused by the global converter memoization which captures the state of the conversion graph at the point in time of the very first invocation for a given pair of source and dest types.

Reproducer:

(defrecord Foo [data])
(defrecord Bar [data])

(defn run [n]
  (prn :begin n)
  (bs/convert (Foo. "foo") String)
  (prn :end n)
  (newline))

(bs/def-conversion ^{:cost 0} [Foo Bar]
  [x _]
  (prn :foo->bar)
  (Bar. (:data x)))

(bs/def-conversion ^{:cost 0} [Bar String]
  [x _]
  (prn :bar->string)
  (:data x))

;; At this point we can only indirectly convert from `Foo` to `String` via `Bar`
(run 1)

;; Now we declare a direct conversion path from `Foo` to `String`
(bs/def-conversion ^{:cost 0} [Foo String]
  [x _]
  (prn :foo->string)
  (:data x))

;; But because the first invocation has already memoized the more costly path, it has no effect
(run 2)

Output:

:begin 1
:foo->bar
:bar->string
:end 1

:begin 2
:foo->bar
:bar->string
:end 2

In contrast, moving both run invocations after the last bs/def-conversion outputs:

:begin 1
:foo->string
:end 1

:begin 2
:foo->string
:end 2

This is a bit of a gotcha which might at least be worth documenting. Alternatively, def-conversion could reset the memo which should solve this issue.

@DerGuteMoritz
Copy link
Collaborator Author

Just found #10 which also touches on the issue of the hidden initialization cost which resulted in the introduction of a precache-conversions API. This didn't solve the early capturing issue but at least is some prior art in the spirit of my suggestion. However, it later got removed again without further explanation. Hm!

@DerGuteMoritz
Copy link
Collaborator Author

The main issue here could also be solved by invalidating the memo whenver new conversions are declared. However, this wouldn't also address the performance gotcha, so I decided to break it out into its own issue.

@KingMob
Copy link
Collaborator

KingMob commented Sep 13, 2022

It's important to articulate the problem; is this a real concern for anyone?

The only non-toy/demo/example I found of using def-conversion anywhere on Github was for clj-fdb, and that was for a new conversion, and in the tests.

This issue is currently theoretical afaict, and I don't think anyone should spend time on it. 😄

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

No branches or pull requests

2 participants