1
\$\begingroup\$

I just started Advent of Code 2018. This is exercise 2 of day 1, and I can't understand why my code is so slow (4 minutes 37 seconds, compared to 200ms for a friend who did it in Clojure with the same computer).

Basically, it accumulates integers from a provided list of ints, named input. Whenever the current value of the accumulator is equal to one of its previous values, it is returned. We might have to loop many times over the provided list before we find such an occurrence. Here is the code:

  let main () =
    let rec my_slow_code lst history =
        if lst == [] then my_slow_code input history
        else
          let current = List.hd lst + List.hd history in
          if List.mem current history then current
          else my_slow_code (List.tl lst) (current :: history)
      in
      my_slow_code input [0]

I can't understand what mistake here makes such a slow code, maybe the use of List.mem?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Dec 12 '18 at 14:49
4
\$\begingroup\$

I won't give it all away, since that would take away some of the fun of Advent of Code, but I'll point out:

First, yes, List.mem takes time proportional to the length of your list, which means if you're calling it over and over again it can be quite slow indeed. There might be faster structures you could use if you want to track if a previous value has been seen before, like Sets. Also, you should not be using == for equality here but rather =; I recommend learning the difference between = and ==, you will want to know it.

Second, your code is not very idiomatic OCaml; in general, if you're doing things like nesting lots of ifs and using List.hd instead of pattern matching, you may not be doing things quite right. This won't impact your performance but might impact readability and correctness. (For example, the match l with | [] -> ... pattern will warn you if a pattern match isn't exhaustive, which avoids nasty bugs.)

\$\endgroup\$
  • \$\begingroup\$ thanks for the tips! Using = instead of == is indeed more correct - even if it did not improve the efficiency. On the other hand, using Set made it much, much faster (more than 500x) \$\endgroup\$ – laurentmeyer Dec 12 '18 at 17:27
  • \$\begingroup\$ It improves it from O(n) to O(1), which is vastly better than improving by a constant, no matter how large. \$\endgroup\$ – Perry Dec 18 '18 at 22:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.