Add ':binding' support #55
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bnert
commented
Mar 30, 2024
bnert
commented
Mar 30, 2024
bnert
commented
Mar 30, 2024
|
Addressed the cleanup items from my previous commit. |
Per metosin#54 in metosin/seippari, this commit could be considered a starting point for supporting bindings support in clojure. The initial implementation used the starting point @robert-stuttaford posted in the referenced issue. A tradeoff here, is that in order to support sync/async w/ the various runtimes, using (bound-fn*) seems to be necessary given the different thread pool implemenations. Counsequently, there is a a perf hit needing to push/pop thread local vars. It doesn't seem to be too much in the general case, but with manifold, it seems to a an order of magnitude slower, though still in the microseconds.
|
@jarppe, pinging here since (from my vantage point), it looks like you're the maintainer. If ya'll would like the contribution, what is your take on these changes? |
Ramblurr
added a commit
to Ramblurr/interceptor
that referenced
this pull request
Nov 13, 2024
This is largely a port of @bnert's PR for sieppari ref: metosin/sieppari#55 The caveats from that PR regarding a small hit to performance, probably apply, though I did not do any benchmarking since the repo isn't setup for it.
|
I asked the folks over at exoscale/interceptor if they would be interested in adding this feature, but they were not (understandably given their scope statement). This leaves sieppari as the only other extant independent interceptor implementation. This feature is needed when trying to interceptorize existing middlewares. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Per #54, this commit could be considered a starting point for supporting bindings support in Clojure, not ClojureScript, given the platform differences.
The initial implementation used the starting point @robert-stuttaford posted in the referenced issue.
Performance Tradeoff
A tradeoff here is in order to support sync/async w/ the various runtimes, using
(bound-fn*)seems to be necessary given the different thread pool implemenations. Counsequently, there looks to be a slight performance hit needing to push/pop thread local vars in the general case. However, manifold seems to a an order of magnitude slower, though still in the microseconds.Not sure the tradeoff ya'll at metosin want to make here. I'll throw in my 2 cents, in that if manifold is being chosen as the async executor, it isn't being chosen for raw performance.
Perf Tests
I ran the following on:
Results of `lein perf-test` before changes
Results of `lein perf-test` after changes