Fork me on GitHub
#cljs-dev
<
2019-07-11
>
bronsa14:07:50

@dnolen when you have a moment could you take a look at https://clojure.atlassian.net/browse/TRDR-58?

bronsa14:07:18

I don't have any knowledge of how this stuff performs in js/is optimised by closure to judge whether the proposed change makes sense

bronsa14:07:58

the proposed change would be going from

(defn- read-string*
  [rdr _ opts]
  (loop [sb (StringBuffer.)
         ch (read-char rdr)]
    (case ch
      nil (err/throw-eof-reading rdr :string \" sb)
      \\ (recur (doto sb (.append (escape-char sb rdr)))
                (read-char rdr))
      \" (str sb)
(recur (doto sb (.append ch)) (read-char rdr)))))
to
(defn- read-string*
  [rdr _ opts]
  (loop [sb ""
         ch (read-char rdr)]  
    (case ch  
      nil (err/throw-eof-reading rdr :string \" sb)  
      \\ (recur (str sb (escape-char sb rdr))  
                (read-char rdr))  
      \" sb  
      (recur (str sb ch) (read-char rdr)))))

bronsa14:07:23

i.e. doing away with StringBuffer in favour of just using str

bronsa14:07:10

the usage of StringBuffer there is just a port of the usage of StringBuilder in the clj impl (where it makes a massive performance difference), but I guess it may be the other way round in cljs

dnolen14:07:48

@bronsa that ticket should probably show throughput so that memory/throughput tradeoff is clear

dnolen14:07:39

but also maybe they should try transit

dnolen14:07:25

also the ticket doesn't show memory usage for a variety of JS engines

dnolen14:07:34

I wouldn't proceed w/ that ticket w/o a lot more information

bronsa14:07:41

cool, I won't do anything until some good analysis is done since I have absolutely no idea what the impact may be in js (nor I have a clue about how to do performance analysis in it), was just wondering if this was an "obvious" improvement to you but sounds like no

bronsa14:07:18

I'll follow up with the original reporter then, thanks

anmonteiro18:07:55

everyone OK with this?

dnolen20:07:34

I don't have any issues with that

dnolen20:07:53

defonce is really a REPL affordance - I don't see how it would be meaningful in advanced

dnolen20:07:35

@anmonteiro so that just removes the existence check right?

dnolen20:07:18

one question I have is what problem that actually fixes in your case?

anmonteiro21:07:13

@dnolen yes the only goal here is to reduce code size

anmonteiro21:07:25

and eliminate the existence check

dnolen21:07:19

@anmonteiro put that in the issue - does that actually cause code not to eliminated?

anmonteiro21:07:27

under advanced

anmonteiro21:07:49

Closure can inline the first case (which would be what my fix does), but not the second

👀 1