Skip to content
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

Refactor Format module to use Queue and Stack #1948

Merged
merged 9 commits into from Aug 8, 2018

Conversation

Projects
None yet
3 participants
@keleshev
Copy link
Contributor

commented Jul 31, 2018

I wanted to contribute a feature to the Format module, but first I decided to refactor it a bit to understand the code better.

Using Stack and Queue modules instead of ad-hoc implementations of stacks and queues improves clarity and reduces the volume of code. I think it makes it easier to contribute to the module.

It looks like Format re-implements queues and stacks on purpose, to avoid dependencies. Perhaps the idea was that someday Stack and Queue would acquire pretty-printing functions that depend on Format. If that is necessary, additional internal modules could be extracted, for example, Stack0 and Queue0, or CamlInternalStack and CamlInternalQueue.

I am interested to hear what you think.

@nojb
Copy link
Contributor

left a comment

Thanks for your contribution!

In general I like the change. I think the use of Queue is a clear win. The case of Stack is a bit less clear because using lists allows to use pattern matching directly which is a little lighter than using Stack.{top, pop} and catching Stack.Empty. But I think it is still worth it.

On the other hand, I don't think the use of the ScanStack functor is justified. Its special behaviour with respect to the usual Stack is only used in one place, in the function set_size, where it is both simpler and easier to see that the patch does not change the existing behaviour to use an usual Stack.t and check for the empty case explicitly. Also, the comments in the code seem to say that the scan_stack is never empty, so, if anything, one should fail if that is found to be the case (I know that is not what the current code does).


(** Stack which is never empty, always contains the default value *)
module DefaultStack (DEFAULT : sig type t val default : t end) : sig
type 'a t

This comment has been minimized.

Copy link
@nojb

nojb Aug 1, 2018

Contributor

The type parameter is always set to DEFAULT.t here. Better to drop it and have just type t.

This comment has been minimized.

Copy link
@nojb

nojb Aug 1, 2018

Contributor

Alternatively, one could define:

type 'a t = 'a * 'a Stack.t
let create : 'a -> 'a t = fun default -> (default, Stack.create ())
let top (default, stack) = try Stack.top stack with Stack.Empty -> default
...
state.pp_mark_close_tag <- mct;
state.pp_print_open_tag <- pot;
state.pp_print_close_tag <- pct
let pp_set_formatter_tag_functions state tag_functions =

This comment has been minimized.

Copy link
@nojb

nojb Aug 1, 2018

Contributor

Here it is better to use pattern matching on the argument to make sure the compiler will let us know if fields are ever added to the argument type.

state.pp_out_newline <- h;
state.pp_out_spaces <- i;
state.pp_out_indent <- j
let pp_set_formatter_out_functions state out_functions =

This comment has been minimized.

Copy link
@nojb

nojb Aug 1, 2018

Contributor

Better to use pattern matching here, as before.

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

Thanks for your review, Nicolás. I will prepare the changes.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

using lists allows to use pattern matching directly which is a little lighter than using Stack.{top, pop} and catching Stack.Empty

Should we add option-returning variants to Stack then? We have a precedent for _opt functions now.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

(Another minor comment (I started reviewing yesterday and did not complete). I think it would be nice to send logically separate changes as separate commits, as it can make reviewing easier and also rebasing as trunk changes. Typically .depend should be on its own, and then you could have had a separate commit for Size, which is independent from the data-structure changes, for example. It's probably not worth splitting your commit after-the-fact, but I hope that you will make similarly nice contributions in the future where it would make sense to split from the start.)

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

Should we add option-returning variants to Stack then? We have a precedent for _opt functions now.

I think this is a very good idea; in fact I was surprised these were not added when the initial batch of *_opt functions were added, as they seem ideal candidates.

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

If no objections, I will create a separate PR for adding Stack.top_opt, Stack.pop_opt, Queue.peek_opt, Queue.take_opt, and once that is merged I will rework this one.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

I think that would be excellent, thanks! (It's a good idea to separate the PRs.)

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Ready for review.

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

AppVeyor:

Build execution time has reached the maximum allowed time for your plan (60 minutes).

@nojb nojb closed this Aug 4, 2018

@nojb nojb reopened this Aug 4, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 4, 2018

You have a "no change entry needed" commit but I believe that your nice factorization work would, in fact, deserve proper credit/recognition through a change entry. Could you add one in either the "Standard library" or "Internal changes" sections?

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

While adding the Change entry I noticed that this PR conflicts with GPR #1731. I will try to resolve that, by avoiding raising Queue.Error. One way would be to use take_opt and peek_opt and then raise_notrace a custom exception, but I hope there will be a more elegant solution.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 4, 2018

Good catch. Note that Empty_queue was only used in a few call-sites, you could just convert them to return an option and not use an exception there at all.

@keleshev keleshev force-pushed the keleshev:refactor-format branch from f3e7eeb to 761b8b4 Aug 5, 2018

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

I removed the use of Queue.Error exception. Now Format doesn't use exceptions for control flow. Also added a Changes entry.

@gasche
Copy link
Member

left a comment

I did a review. Overall this is very good, and my comments are only nitpick -- you like to write good code, so I thought you could be interested in extra opinions about it.

There is one real question in a situation where I can't tell whether the change is correct, namely the Pp_close_tag case of the main format_pp_token function.

| first :: _ ->
let rec find = function
| head :: _ when head >= insertion_point -> head
| _ :: tail -> find tail

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

This is pure nitpick, but personally I tend dislike when and think that it is better to use it only when it brings large benefits in readability. (I think that in most other cases, the implicit dependencies it adds between the cases offset the advantages.) Here, I would say that the fact that your two lines each ignore a part of their pattern is a sort of code smell suggesting that an if would be nicer.

let marker = state.pp_mark_close_tag tag_name in
pp_output_string state marker;
state.pp_mark_stack <- tags

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

Real Question: with the old version of the code, the mark was only removed from the state after pp_output_string had run. Is it possible that this would affect the behavior? (I'm not familiar with the codebase.)

This comment has been minimized.

Copy link
@keleshev

keleshev Aug 6, 2018

Author Contributor

As I was drafting a response to you about how this would never affect the behavior, I actually… found a case where it affects the behavior.

pp_output_string delegates to state.pp_out_string. Built-in formatters set state.pp_out_string to Pervasives.output_substring or Buffer.add_substring. However, it could be set to anything, because it can be user-provided (by calling pp_make_formatter, for example). So if the user sets it to something that calls, for example, pp_open_tag on the same formatter, then:

  • In the old code, the open tag would have been lost, overriden by the <- tags.
  • In the new code, the open tag would be there, but handled before the close tag.

Neither of these make any sense, because it is such a contrived example.

In general, calling "porcelain" functions like pp_open_tag inside "plumbing" functions like "pp_output_string" would definitely wreak havoc in the internal state of the formatter. So I consider leaving this as it is.

This comment has been minimized.

Copy link
@gasche

gasche Aug 6, 2018

Member

Ok. I'm convinced by the explanation. In general I buy the idea that popping early probably preserves the structure invariants better than popping late.

let rec add_tab n = function
| [] -> [n]
| x :: l as ls -> if n < x then n :: ls else x :: add_tab n l in
tabs := add_tab (state.pp_margin - state.pp_space_left) !tabs
| [] -> () (* No open tabulation box. *)
| None -> () (* No open tabulation box. *)

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

Minor nitpick: I tend to think that a conditional or match is more readable when the shortest case is handled first, so I would have the None clause first in these cases.

match Queue.peek_opt state.pp_queue with
| None -> () (* No tokens to print *)
| Some { size; token; length } ->
let is_size_known = Size.is_known size in

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

Not convinced that the is_size_known binding makes the code more readable, it's mostly transparent.

@@ -794,7 +701,7 @@ let pp_close_tbox state () =
if state.pp_curr_depth > 1 then
begin
if state.pp_curr_depth < state.pp_max_boxes then
let elem = make_queue_elem (size_of_int 0) Pp_tend 0 in
let elem = { size = Size.zero; token = Pp_tend; length = 0 } in

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

We had many { size = Size.zero; token; length = 0 } repetitions already, maybe you could use an auxiliary function?

let zero_token token = { size = Size.zero; token. length = 0 }

This comment has been minimized.

Copy link
@keleshev

keleshev Aug 6, 2018

Author Contributor

I tried zero_token, but settled on re-introducing make_queue_elem but with optional size and length defaulting to zero. I rewrote the history a bit to hide the back-and-forth.

If I ever come up with a good name for "pretty printing queue element" (which is different from token), I will probably introduce a module for it with make_queue_elem becoming create.

This comment has been minimized.

Copy link
@gasche

gasche Aug 6, 2018

Member

Actually I'm not fond of this new approach, because it makes it easy for programmers to write make_queue_elem and not think about the size and length they want for their token. Explicit record initialize forced them to think, and zero_token has a name that clearly indicates that it should only be used in some special situation.

If you had a systematic way to compute the size and length from the token (it looks like it might be the case from the code), then you could make those arguments go away from make_queue_elem in a way that is safe, because it always make the right choice. But hiding a choice that can be wrong is not good UI.

{ size = Size.unknown; token = Pp_begin (0, Pp_hovbox); length = 0 } in
Queue.add sys_tok pp_queue;
let scan_stack = Stack.create () in
initialize_scan_stack scan_stack;

This comment has been minimized.

Copy link
@gasche

gasche Aug 5, 2018

Member

Nitpick:

let scan_stack = initialize_scan_stack (Stack.create ())

would be nicer, as it does not give a name to an invalid stack. (One could also have a function for this.)

This comment has been minimized.

Copy link
@keleshev

keleshev Aug 6, 2018

Author Contributor

I think this works nice at this particular call site, but doesn't work as well at the other two call sites, where initialize_scan_stack is used to re-initialize an existing stack.

This comment has been minimized.

Copy link
@gasche

gasche Aug 6, 2018

Member

I agree that it wouldn't work for other use-sites of initialize_scan_stack, my point was that let scan_stack = Stack.create () gives a stack name to something that is not a valid stack. (This minor glitch does not occur in the other callsites of initialize.)

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

( @nojb I think your "requested changes" have been addressed so you could probably dismiss/change your earlier review. )

state.pp_left_total <- state.pp_left_total - len;
state.pp_space_left <- state.pp_space_left + int_of_size size
match Queue.take_opt state.pp_queue with
| None -> () (* print_if_newline must have been the last printing command *)

This comment has been minimized.

Copy link
@nojb

nojb Aug 5, 2018

Contributor

Previous comment indicates that this case cannot happen; new comment indicates that it can. Could you say a few words to understand the discrepancy ?

This comment has been minimized.

Copy link
@keleshev

keleshev Aug 5, 2018

Author Contributor

I had some explanation in the commit message:

Function pp_skip_token had a comment saying that the queue is never empty when it is called, and it was tempting to rely on that. However, I came up with a unit test that falsifies that invariant, see pp_skip_token.ml. This prompted to change pp_skip_token to gracefully handle the case when the queue is empty. Before, the invariant was wrong, but the code still worked correctly because the exception would have been caught in advance_left.

This comment has been minimized.

Copy link
@nojb

nojb Aug 5, 2018

Contributor

Thanks, I had missed that!

@nojb

nojb approved these changes Aug 5, 2018

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2018

Thanks for the review comments. I think they are worth addressing, so don't merge it yet!

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

I have a minor comment: another (new) way to get performance-free type abstraction for size would be

type size = Size of int [@@unboxed]

This has the advantage of being usable in pattern-matching (so many uses of Size.to_int could be converted to a pattern-matching, making the code more elegant), and the inconvenient of not giving a natural place to define known and is_known -- but you could also define them as auxiliary functions at the toplevel. Personally I think it would be slightly nicer (ligther) than the current submodule approach, only very slightly: I don't think it's worth the work required to make the change.

@keleshev keleshev force-pushed the keleshev:refactor-format branch from 761b8b4 to 14ae9e3 Aug 6, 2018

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

The Size of int [@@unboxed] is really neat and reduces some volume of code. But I want to hide negative integers as much as possible within the Size module. I haven't completely succeeded in this, but it's a first step.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

@keleshev: I feel like we are at a rather stable point where we can always discuss further changes to make, but generally the thing is good. I would be in favor of moving to merge this, but I would need two things:

  1. There is a fair amount of changes in the commit set that don't need to be kept forever in history, it would be nice to rebase them to keep atomic changes but remove back-and-forth, merges etc.

  2. Because this touches the fundamental datastructures of the Format module, and I believe some of the original implementation choices (eg. the somewhat weird way size was made an abstract type) were done for performance reasons, I would find it reassuring to see a small micro-benchmark to see that there aren't dramatic changes in the library's performance. This wouldn't need to be very elaborate, but a loop formatting some simple output with a couple boxes would be nice. Would you have the time to test that?

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

(Also, you need to be happy with the current state at a given point, and signal it's ready to merge.)

keleshev added some commits Aug 3, 2018

Format: introduce Size module
Use proper abstract module instead of "%identity" trick.
Add Size.unknown and Size.is_unknown to aid readability
instead of relying on a comment saying that negative
sizes mean unkown sizes.
Format: replace bespoke queue for Stdlib.Queue
This reduces the volume of code in Format and adds clarity by using a
familiar Queue interface. However, this introduces a dependency from
Format to Queue.

Note, `state.pp_queue` was unnecessarily mutable.

keleshev added some commits Aug 3, 2018

Format: replace inline list stacks with Stdlib.Stack
This reduces the volume of code in Format and adds clarity by using a
familiar Stack interface. However, this introduces a dependency from
Format to Stack.
Format: rename pp_queue_elem.elem_size to size
The original name is tautological, plus the new name works better with
record field punning.
Format: remove make_queue_element
Since it doesn't bring anything to the table. Use record constructor instead.

@keleshev keleshev force-pushed the keleshev:refactor-format branch from 57be0fa to a2a0266 Aug 7, 2018

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Benchmark

let fprintf = Format.fprintf

let () = Random.init 42

type xml = String of string | Tag of string * xml list

let rec format_xml f = function
  | String text -> fprintf f "%s" text
  | Tag (name, body) ->
      let format_body = Format.pp_print_list format_xml in
      fprintf f "@[<hv 3><%s>@,%a@;<0 -3></%s>@]" name format_body body name

let random_list random_element =
  Array.make (Random.int 4) () |> Array.map random_element |> Array.to_list
let random_string () = [|"foo"; "baz"; "bar"|].(Random.int 3)

let rec random_xml () =
  match Random.int 2 with
  | 0 -> String (random_string ())
  | 1 -> Tag (random_string (), random_list random_xml)
  | _ -> assert false

let corpus = Array.make 100_000 () |> Array.map random_xml

let () =
  corpus |> Array.iter (fun xml ->
    fprintf Format.std_formatter "%a@," format_xml xml)

Trunk vs. this branch vs. this branch with Size module reverted

$ hyperfine --warmup=1 --min-runs=10 ./trunk.out ./refactor-format.out ./no-module-size.out 
Benchmark #1: ./trunk.out

  Time (mean ± σ):     587.2 ms ±   6.2 ms    [User: 553.2 ms, System: 24.6 ms]
 
  Range (min … max):   579.2 ms … 597.2 ms
 
Benchmark #2: ./refactor-format.out

  Time (mean ± σ):     612.6 ms ±   7.1 ms    [User: 576.7 ms, System: 26.5 ms]
 
  Range (min … max):   597.7 ms … 623.4 ms
 
Benchmark #3: ./no-module-size.out

  Time (mean ± σ):     610.0 ms ±   8.3 ms    [User: 573.9 ms, System: 27.0 ms]
 
  Range (min … max):   599.0 ms … 627.2 ms
 
Summary

  './trunk.out' ran
    1.04x faster than './no-module-size.out'
    1.04x faster than './refactor-format.out'

My guess is that the 4% difference is due to one more pointer of indirection for each stack.

keleshev added some commits Aug 4, 2018

Format: make `pp_skip_token` gracefully handle empty queue
Function `pp_skip_token` had a comment saying that the queue is never empty
when it is called.  However, I came up with a unit test that falsifies that
invariant, see `pp_skip_token.ml`. This prompted to change `pp_skip_token` to
gracefully handle the case when the queue is empty. Before, the invariant was
wrong, but the code still worked correctly because the exception would have
been caught in `advance_left` which used to use `Queue.Empty` exception for
control flow.

@keleshev keleshev force-pushed the keleshev:refactor-format branch from a2a0266 to c4990a4 Aug 8, 2018

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

I am pretty happy with the state of this PR right now. The last rebase only reworded a commit message.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

Looks good to me, I'll merge after lunch. Thanks for your nice work!

@keleshev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

@gasche @nojb thanks for the collaboration. The PR looks much better now than when I submitted it.

@gasche

gasche approved these changes Aug 8, 2018

@gasche gasche merged commit ed97c44 into ocaml:trunk Aug 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@keleshev keleshev deleted the keleshev:refactor-format branch Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.