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

Compatibility with ring/ring-codec #1091

Closed
FieryCod opened this issue Dec 6, 2021 · 7 comments
Closed

Compatibility with ring/ring-codec #1091

FieryCod opened this issue Dec 6, 2021 · 7 comments

Comments

@FieryCod
Copy link
Contributor

FieryCod commented Dec 6, 2021

Is your feature request related to a problem? Please describe.
I would love to be able to use ring/ring-codec in holy-lambda-ring-adapter, but unfortunately java.util.StringTokenizer is not available in babashka.

Describe the solution you'd like
May I please propose a PR in which I would add java.util.StringTokenizer to a list of supported classes?

Describe alternatives you've considered
Making a fork of ring-codec without StringTokenizer.

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

You can see an earlier commit in the history of ring-codec in which I made it compatible with babashka. It's a pity that this commit has made it incompatible again:

ring-clojure/ring-codec@92a2d66#diff-e42d7d4760cc0ec1368c75ec5ec79429d64d6795c32717c6576540162ceffc9a

/cc @bsless

The paradox with babashka is that lower level code like this is usually slower (or incompatible) with bb than using high level constructs since there is more interpretation overhead.

Perhaps we can just maintain a fork of this library or introduce a reader conditional into the main repo, but I'm not sure if the original maintainer is OK with that.

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

However, by adding this class, it will make things compatible again. I've tested it locally:

(prn (reduce (fn [m param]
               (prn param) m)
             {} (tokenized "foo&bar&baz" "&")))
"foo"
"bar"
"baz"
{}

I'll make a branch and let CI run it to see what the increased binary size would be.

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

Benchmark:

(time
 (dotimes [_ 100000]
   (vec (tokenized "foo&bar&baz" "&"))))

(time
 (dotimes [_ 100000]
   (str/split "foo&bar&baz" #"&")))
"Elapsed time: 1954.26187 msecs"
"Elapsed time: 80.096692 msecs"

I'm ok with adding the class, since it doesn't take a lot of space, but it does seem to degrade performance somewhat.

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

Merged: #1092

@borkdude borkdude closed this as completed Dec 6, 2021
@FieryCod
Copy link
Contributor Author

FieryCod commented Dec 6, 2021

Thank you!

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

Btw, there are some optimizations to be made with respect to interop. The method to invoke can be detected at analysis time rather than run time. This would speed up things considerably for this example, I think.

@borkdude
Copy link
Collaborator

borkdude commented Dec 6, 2021

See babashka/sci#516 and babashka/sci#644.

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