-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Speed boost by removing clj->js
and using transducers where applicable
#133
Conversation
Sorry - I didn't note that the Phantom tests are failing. Fix comes ASAP. |
@the-kenny I am AFK at the moment. I'll take a look at this over the weekend. Thanks! |
@the-kenny Could you add your before/after comparisons of your rendering process? |
@the-kenny I tested this on one of my projects. Looks good. Could you please rebase on master. I changed the tests to use Tubax instead of Hickory. Thanks! |
@r0man Rebase will come in a minute - not sure if I can get to before/after comparisons today. I'll add them asap. |
Most notably: - Replace most usages of nested map/filter/zip-map with transducers - Get rid of `clj->js` in `sablono.interpreter/attributes`
d1239df
to
3efe976
Compare
I rebased and commited this to master. Thanks! |
Awesome! :) |
|
||
(defn compile-attrs | ||
"Compile a HTML attribute map." | ||
[attrs] | ||
(->> (seq attrs) | ||
(map #(apply compile-attr %1)) | ||
(apply merge) | ||
(reduce (fn [attrs [name value]]some |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the some
at the end of the line doing?
This Pull Request implements various small changes which will in sum improve the performance of
interpret
massively.In our application Pepa we saw slow rendering times dominated by sablono's usage of
clj->js
insablono.interpreter/attributes
. While learning about the internals of sablono I noticed various sub-optimal usages of sequence APIs.While the major gain comes from the removal of
clj->js
, the other parts should add up to something to: The transducers avoid many small allocations for LazySeqs, especially noticeable in chained sequence operations.All tests pass, and our Pepa works fine with these changes. However, I can't rule out that the removal of
clj->js
will cause semantic changes. However, these should only be noticeable in exotic usages of attribute-maps, so I doubt the majority of people should see any change at all (except for better performance of course).You can test out my changes by using
[de.tarn-vedra/sablono "0.7.6-SNAPSHOT"]
If you're interested I can also post some before/after comparisons of our rendering process, but these will just be micro-benchmarks where I take a screenshot of Chrome's flame-graph before and after my changes :-)