Improve iolist optimization #94
Conversation
The new function `optimize_iolist` combines the old `merge_binaries` and `simplify_iolist` functions. It also improves on them slightly, for instance by terminating iolists with a binary instead of the empty list, and by more consistently combining adjacent binaries.
|
|
||
| ["a", "b", ["c", [var]]] => ["abc", var] | ||
| [<<1>>, <<2>>] => <<1, 2>> |
There was a problem hiding this comment.
You could make this a full doctest.
There was a problem hiding this comment.
That's a good thought. It's tricky to represent nicely as runnable code though, since it works on quoted expressions. That's why UtilsTest uses a macro.
Really, these functions are not something I would expect people to try to re-use. Or I hope that don't. Maybe we should @moduledoc false this whole thing.
There was a problem hiding this comment.
That makes sense. You can use standard comments to describe what's going on here for internal consumption.
| @@ -60,30 +60,47 @@ defmodule Thrift.Generator.Utils do | |||
| end | |||
|
|
|||
| @doc """ | |||
| Merge the binaries in an iolist. | |||
| Optimize an expression that returns an iolist. | |||
There was a problem hiding this comment.
Could you also write a little more about the optimizations that are applied while they're still fresh in your mind?
| optimize_iolist([a, b | rest]) | ||
| end | ||
| def optimize_iolist([a, b]) do | ||
| [{:|, [], [a, b]}] |
There was a problem hiding this comment.
I'm not sure I understand this return result. What's going on in this case?
There was a problem hiding this comment.
A list is represented in memory as a linked list of cons cells. A cons cell is a pair of adjacent values [head | tail] where head is the value at that point in the list and tail is either the next cons cell or the empty list. For example, a list with three elements looks like:
["a" | ["b" | ["c" | []]]]
An iolist is also composed out of cons cells but one less constraint: the tail can also be a binary. This means you can save one cons when constructing an iolist. The same list above could be converted to the iolist:
["a" | ["b" | "c"]]
It uses slightly less memory. It's also slightly faster to traverse, as there's one less pointer to follow. This optimization is performed by the above optimize_iolist clause.
There was a problem hiding this comment.
Thanks, I see now. I wasn't connecting with the fact that this is entirely based on quoted expressions.
| @@ -0,0 +1,23 @@ | |||
| defmodule Thrift.Generator.UtilsTest do | |||
| use ThriftTestCase | |||
There was a problem hiding this comment.
Can this just use ExUnit.Case?
| # Optimize a quoted expression that returns an iolist. The high level strategy | ||
| # is to flatten lists and combine adjacent binaries. | ||
| # | ||
| def optimize_iolist(expr=[[a | b] | c]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "flatten list") | ||
| optimize_iolist([a, b | c]) | ||
| end | ||
| def optimize_iolist(expr=[a, [b | c] | d]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "flatten list") | ||
| optimize_iolist([a, b, c | d]) | ||
| end | ||
| def optimize_iolist(expr=[[] | a]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "discard empty list") | ||
| optimize_iolist(a) | ||
| end | ||
| def optimize_iolist(expr=[a, [] | b]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| end | ||
| def merge_binaries([a, b | rest]) when is_list(b) do | ||
| merge_binaries([a] ++ b ++ rest) | ||
| def optimize_iolist(expr=[a, b, [] | c]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| end | ||
| def merge_binaries([{:<<>>, [], a}, {:<<>>, [], b} | rest]) do | ||
| merge_binaries([{:<<>>, [], a ++ b} | rest]) | ||
| def optimize_iolist(expr=[{:|, _, a} | b]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| end | ||
| def merge_binaries([a | rest]) do | ||
| [a] ++ merge_binaries(rest) | ||
| def optimize_iolist(expr=[a, {:|, _, b} | c]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| end | ||
| def merge_binaries(a) do | ||
| def optimize_iolist(expr=[{:<<>>, opts, a}, {:<<>>, _, b} | rest]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "merge binary expressions") | ||
| optimize_iolist([{:<<>>, opts, a ++ b} | rest]) | ||
| end | ||
| def optimize_iolist(expr=[a, {:<<>>, opts, b} | rest]) when is_binary(a) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "merge binary and binary expression") | ||
| optimize_iolist([{:<<>>, opts, [a | b]} | rest]) | ||
| end | ||
| def optimize_iolist(expr=[{:<<>>, opts, a}, b | rest]) when is_binary(b) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "merge binary expression and binary") | ||
| optimize_iolist([{:<<>>, opts, a ++ [b]} | rest]) | ||
| end | ||
| def optimize_iolist(expr=[a, b | rest]) when is_binary(a) and is_binary(b) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "merge binaries") | ||
| optimize_iolist([a <> b | rest]) | ||
| end | ||
| def optimize_iolist(expr=[a, b]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
| debug_optimization(expr, "final cons cell") | ||
| [{:|, [], [a, b]}] | ||
| end | ||
| def optimize_iolist(expr=[a]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
|
|
||
| def simplify_iolist([{:<<>>, _, _} = binary]) do | ||
| binary | ||
| def optimize_iolist(expr=[a | rest]) do |
There was a problem hiding this comment.
There are spaces around operators most of the time, but not here.
The new function
optimize_iolistcombines the oldmerge_binariesandsimplify_iolistfunctions. It also improves on them slightly, for instance byterminating iolists with a binary instead of the empty list, and by more
consistently combining adjacent binaries.