ClojureScript

Optimize js->clj by switching to transients

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Trivial Trivial
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Activity

Hide
David Nolen added a comment -

Did you any benchmarks on other JS engines?

Show
David Nolen added a comment - Did you any benchmarks on other JS engines?
Hide
Darrick Wiebe added a comment -

No. My main concern was to check if there was any startup overhead that might offset the basic efficiency improvements for processing small collections. Using into with a transducer turned out to be faster in every scenario, significantly in most.

Show
Darrick Wiebe added a comment - No. My main concern was to check if there was any startup overhead that might offset the basic efficiency improvements for processing small collections. Using into with a transducer turned out to be faster in every scenario, significantly in most.
Hide
David Nolen added a comment - - edited

It would be nice to see a jsperf of before and after on this ticket. Thanks.

Show
David Nolen added a comment - - edited It would be nice to see a jsperf of before and after on this ticket. Thanks.
Hide
Darrick Wiebe added a comment -

Is there a existing one that I can work from?

Show
Darrick Wiebe added a comment - Is there a existing one that I can work from?
Hide
David Nolen added a comment -

There is not. I generally just make a simple project, create an advanced compiled artifact and copy and paste it into jsperf.

Show
David Nolen added a comment - There is not. I generally just make a simple project, create an advanced compiled artifact and copy and paste it into jsperf.
Hide
Darrick Wiebe added a comment -

Turns out reducing into a transient is considerably better than using a transducer (which was itself a little faster) for this.

http://jsperf.com/js-clj-transducer-test

The code is at:

https://gist.github.com/pangloss/591d77231fda460c2fbe

Let me know if you want me to prepare an updated patch.

Show
Darrick Wiebe added a comment - Turns out reducing into a transient is considerably better than using a transducer (which was itself a little faster) for this. http://jsperf.com/js-clj-transducer-test The code is at: https://gist.github.com/pangloss/591d77231fda460c2fbe Let me know if you want me to prepare an updated patch.
Hide
David Nolen added a comment -

Thanks for putting this together. Yes please provide an updated patch.

Show
David Nolen added a comment - Thanks for putting this together. Yes please provide an updated patch.
Hide
Darrick Wiebe added a comment -

Not sure whether the convention is to comment that I've uploaded a new patch. Regardless, I uploaded it yesterday.

Show
Darrick Wiebe added a comment - Not sure whether the convention is to comment that I've uploaded a new patch. Regardless, I uploaded it yesterday.
Hide
Marcin Kulik added a comment -

I have tested and benchmarked the patch on a big js objects (5MB+ json files) and I confirm that new-js->clj3 function is more than 2x faster. It runs now in the player on https://asciinema.org and all seems good so far.

Show
Marcin Kulik added a comment - I have tested and benchmarked the patch on a big js objects (5MB+ json files) and I confirm that new-js->clj3 function is more than 2x faster. It runs now in the player on https://asciinema.org and all seems good so far.
Hide
David Nolen added a comment - - edited

Marcin thanks for the feedback. Experience reports always help push tickets forward.

Darrick, the patch need to be rebased to master. Please remove all patches except the one that will be applied.

Show
David Nolen added a comment - - edited Marcin thanks for the feedback. Experience reports always help push tickets forward. Darrick, the patch need to be rebased to master. Please remove all patches except the one that will be applied.
Hide
Rohit Aggarwal added a comment - - edited

[ClojureScript beginner here. so do ignore this if I am mistaken]

The patch of 25/Aug uses `(aget x k)` instead of `(goog.object/get x k)` for getting the value of a key if x is an object. I believe it isn't the right way even though it works.

This contains why the latter is preferred over the former.

Show
Rohit Aggarwal added a comment - - edited [ClojureScript beginner here. so do ignore this if I am mistaken] The patch of 25/Aug uses `(aget x k)` instead of `(goog.object/get x k)` for getting the value of a key if x is an object. I believe it isn't the right way even though it works. This contains why the latter is preferred over the former.
Hide
David Nolen added a comment -

Rohit good catch. Yes that should be changed.

Show
David Nolen added a comment - Rohit good catch. Yes that should be changed.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: