refactor: re-implement [Appendable_list]

[rgrinberg]

This is the same as #9033 without any additional book-keeping for empty checks or using the most compact constructors. Performance is looking better now.

[pmwhite]

Looks good to me. I'm not suggesting we put it in this PR, but I'm curious if providing built-in iteration functions for Appendable_list.t would be worthwhile. They would save the extra iteration and allocation that to_list does, but each iteration would probably be more expensive because of all the indirection in the data structure. I guess it depends on how many times we iterate over the list of aliases.

[pmwhite]

Actually, it occurs to me that we can reduce allocation in to_list_rev a bit:

let to_list_rev =
  let rec loop1 acc t stack =
    match t with
    | Empty -> loop0 acc stack
    | Singleton x -> loop0 (x :: acc) stack
    | Cons (x, xs) -> loop1 (x :: acc) xs stack
    | List xs -> loop0 (List.rev_append xs acc) stack
    | Append (xs, ys) -> loop1 acc xs (ys :: stack)   
    | Concat [] -> loop0 acc stack
    | Concat (x :: xs) -> loop1 acc x (Concat xs :: stack)
  and loop0 acc stack =                                 
    match stack with
    | [] -> acc
    | t :: stack -> loop1 acc t stack
  in
  fun t -> loop1 [] t []    
;;

[rgrinberg]

Looks good to me. I'm not suggesting we put it in this PR, but I'm curious if providing built-in iteration functions for Appendable_list.t would be worthwhile. They would save the extra iteration and allocation that to_list does, but each iteration would probably be more expensive because of all the indirection in the data structure. I guess it depends on how many times we iterate over the list of aliases.

One issue that makes this idea less attractive is that we are specifically converting this into a list because the list is a better representation for memoization (uses less space, easy to compare/hash).

[rgrinberg]

Actually, it occurs to me that we can reduce allocation in to_list_rev a bit:

let to_list_rev =
  let rec loop1 acc t stack =
    match t with
    | Empty -> loop0 acc stack
    | Singleton x -> loop0 (x :: acc) stack
    | Cons (x, xs) -> loop1 (x :: acc) xs stack
    | List xs -> loop0 (List.rev_append xs acc) stack
    | Append (xs, ys) -> loop1 acc xs (ys :: stack)   
    | Concat [] -> loop0 acc stack
    | Concat (x :: xs) -> loop1 acc x (Concat xs :: stack)
  and loop0 acc stack =                                 
    match stack with
    | [] -> acc
    | t :: stack -> loop1 acc t stack
  in
  fun t -> loop1 [] t []    
;;

Thanks. I applied this patch.

[rgrinberg]

@pmwhite @snowleopard let me know once this is benchmarked internally.

[snowleopard]

I think we should disentangle the changes to the Appendable_list implementation from the rest of the refactoring, to make it easier to benchmark.

[snowleopard]

OK, I think this one is good to go, though I don't know how our internal benchmarking went.

[snowleopard]

OK, I got some benchmark results for this PR applied internally: it's a consistent but small improvement (0.3-0.4% time, 0.1% memory). Let's merge it.

@pmwhite I'm assuming you're going to import it as part of your feature. (Though it's easy for me to do too since I've got a feature ready.)