Let `File.write` and `File.write!` take advantage of `writev` #5154

nathanl · 2016-08-24T14:38:04Z

As a performance optimization, have these functions use :file.write/3,
usually with :raw mode, instead of :file.write_file/3. This will
cause the BEAM to use the writev system call instead of write.

The advantage of this is that, when writing an iolist, the BEAM need not
concatenate the items and produce a single binary for writing. Instead,
it can let the operating system read each item from memory individually;
the final output is produced only in the target file. Skipping the
binary concatenation saves work, saves memory, and creates less garbage
for later collection.

Note that :raw mode cannot be used when a character encoding is given.
In :raw mode, the BEAM does not create a separate process to handle
the file, and (I think) it would normally be this process which performs
the character encoding. So if an encoding like :utf8 is given, we do
not use :raw mode and therefore do not use writev.

As a performance optimization, have these functions use `:file.write/3`, usually with `:raw` mode, instead of `:file.write_file/3`. This will cause the BEAM to use the `writev` system call instead of `write`. The advantage of this is that, when writing an iolist, the BEAM need not concatenate the items and produce a single binary for writing. Instead, it can let the operating system read each item from memory individually; the final output is produced only in the target file. Skipping the binary concatenation saves work, saves memory, and creates less garbage for later collection. Note that `:raw` mode cannot be used when a character encoding is given. In `:raw` mode, the BEAM does not create a separate process to handle the file, and (I think) it would normally be this process which performs the character encoding. So if an encoding like `:utf8` is given, we do not use `:raw` mode and therefore do not use `writev`.

nathanl · 2016-08-24T14:40:00Z

lib/elixir/lib/file.ex

@@ -1340,6 +1355,28 @@ defmodule File do
  defp normalize_modes([], true), do: [:binary]
  defp normalize_modes([], false), do: []

+  defp normalize_write_modes(modes) do
+    normalize_modes([:write] ++ modes, true)


Here I'm setting :binary because, even though I'm not sure it's necessary, it's more like what the code used to do. We previously used :file.write_file/3, and the docs say that "the mode flags binary and write are implicit" when calling that function.

josevalim · 2016-08-24T16:05:38Z

lib/elixir/lib/file.ex

+  # and raw mode is also requested.
+  # http://erlang.org/doc/man/file.html#open-2
+  defp set_raw_unless_encoding_specified(modes) do
+    case specifies_encoding?(modes) do


Keyword.has_key?(modes, :encoding)?

Oh, interesting. Docs say "A keyword is a list of two-element tuples" - I didn't know that would work if not all the elements in the list were tuples.

Probably List.keymember?/3 will better communicate what data structure we are working with here (even if it's totally the same as Keyword.has_key?/2 technically).

lexmag · 2016-08-24T16:33:18Z

lib/elixir/lib/file.ex

+  # and raw mode is also requested.
+  # http://erlang.org/doc/man/file.html#open-2
+  defp set_raw_unless_encoding_specified(modes) do
+    case Keyword.has_key?(modes, :encoding) do


This case looks like an if.

I debated about that. This was one line shorter and seemed readable, but I'm happy to change it if you prefer.

How about:

if Keyword.has_key?(modes, :encoding), do: modes, else: [:raw | modes]

I prefer the if as well :)

K, changing. 😄

fishcakez · 2016-08-24T16:46:42Z

I don't understand how this is different to using :file.write_file/3 with :raw mode added?

Note that there is a comment in :file saying :file.write_file/3 should be moved to the file server to be more efficient. Whereas :file.write_file/2 is carried out on the file server using the equivalent of raw mode.

nathanl · 2016-08-24T16:52:48Z

I don't understand how this is different to using :file.write_file/3 with :raw mode added?

Good question. I agree that it shouldn't be different, but it seems to be. If I do this in iex:

:file.write_file("/tmp/tmp.txt", ["foo", "bar"], [:raw])

...and watch it with Evan Miller's dtrace script, I see it using write. But if I do this:

{:ok, file}  = :file.open("/tmp/tmp.txt", [:write, :raw])
:file.write(file, ["foo", "bar"])

... it uses writev. I don't know why this difference exists.

josevalim · 2016-08-24T16:53:33Z

@fishcakez write_file is converting everything into a binary before hand.

That said, @nathanl, we should convert everything to a binary if a encoding was given (i.e. we are not in raw mode).

fishcakez · 2016-08-24T16:55:42Z

Ah this is a bug in :file, shouldn't call make_binary/1 if raw file.

josevalim · 2016-08-24T16:56:35Z

Ah this is a bug in :file, shouldn't call make_binary/1 if raw file.

Yup, that's pretty much it. Should we instead fix Erlang/OTP?

nathanl · 2016-08-24T16:57:50Z

That said, @nathanl, we should convert everything to a binary if a encoding was given (i.e. we are not in raw mode).

Because we don't trust Erlang's UTF-8 encoding? I added a test for the conversion of charlists, but noticed that Erlang complained if I gave it a larger codepoint, like 9731 (☃).

fishcakez · 2016-08-24T16:58:03Z

Yeah I think we should fix this in Erlang/OTP whatever we do here.

josevalim · 2016-08-24T16:58:20Z

Yeah I think we should fix this in Erlang/OTP whatever we do here.

Ok, we can fix it in here meanwhile.

fishcakez · 2016-08-24T16:59:33Z

Ok, we can fix it in here meanwhile.

Sounds good.

nathanl · 2016-08-24T17:00:12Z

Yeah I think we should fix this in Erlang/OTP whatever we do here.

That would be great. When we're ready to merge this, I'll make sure that the relevant change is one commit so that it can be easily reverted later if Erlang's bug is fixed.

fishcakez · 2016-08-24T17:00:36Z

lib/elixir/lib/file.ex

+      {:ok, file} ->
+        case F.write(file, content) do
+          :ok ->
+            F.close(file)


Please check :file.write_file and copy the close handling.

@fishcakez Pushed a commit, copying what I see at https://github.com/erlang/otp/blob/5492ce9951aced8686dbef99d0693e7c6da50c7d/lib/kernel/src/file.erl#L400-L419

josevalim · 2016-08-24T17:08:29Z

Because we don't trust Erlang's unicode conversion?

write_file expects IO data. So there is no unicode conversion happening here. I would expect any list with more than > 255 to fail. The reason we convert to binary is because when raw mode is not used, we spawn another process, and sending the binary to the other process will be more performant. The conversion can be done by calling IO.iodata_to_binary.

Also, when not using raw mode, convert data to binary.

nathanl · 2016-08-24T18:44:27Z

Based on @fishcakez feedback, this no longer sets :raw mode automatically. So we do now get the advantage of writev, but only if the user does File.write("/tmp/tmp.txt", [foo, bar, foo], [:raw]).

See https://github.com/erlang/otp/blob/5492ce9951aced8686dbef99d0693e7c6da50c7d/lib/kernel/src/file.erl#L400-L419

nathanl · 2016-08-24T19:14:50Z

Once all concerns are addressed, I'll squash this down and revise the commit message. Like I said, I'll make the code changes one commit so that it can easily be reverted once :file.write_file is fixed in Erlang/OTP.

josevalim · 2016-08-24T20:59:25Z

@nathanl, at this point, given we have basically reimplemented file:write_file, I believe we should send this fix directly to Erlang/OTP. The reason why I was OK with previous patch was because we were defaulting to raw but @fishcakez convinced us it was a bad idea.

That said, I don't believe we should merge this, sorry. On the positive side, there is no way we could get to this conclusion without your patch and work, so the work here is definitely not being wasted. Let me know if you need help in providing a patch to Erlang/OTP or if you prefer me to do it.

Thank you!

nathanl · 2016-08-24T21:06:40Z

@josevalim No worries! I think I will give it a try, but I may come back for help, since I've not written any Erlang yet.

Do I understand correctly that this line is where they convert the input to a binary, and that should be changed to only happen if raw is not in the list of modes?

josevalim · 2016-08-24T22:59:31Z

Do I understand correctly that this line is where they convert the input to a binary, and that should be changed to only happen if raw is not in the list of modes?

Yes!

josevalim · 2016-08-26T14:34:26Z

@nathanl feel free to ping me on IRC, during the conf, or by e-mail any time then! Closing this as mentioned!

nathanl · 2016-08-26T18:57:56Z

@josevalim and @fishcakez Please see my PR on Erlang/OTP at erlang/otp#1149

josevalim · 2016-08-27T03:45:05Z

@nathanl thank you, it looks great. Btw, Erlang indentation rules is a bit weird, you may have to amend your patch for that. They use 4 spaces indentation but every 8 spaces becomes a tab.

nathanl · 2016-09-29T13:24:43Z

@josevalim @fishcakez My PR on OTP was merged!! 😲 🎊 🎆 ❗

josevalim · 2016-09-29T14:20:18Z

@nathanl yay! congratulations! 🎉

fishcakez · 2016-09-29T14:47:17Z

@nathanl nice work ❤️

nathanl added 2 commits August 24, 2016 09:55

Add a test for writing charlists as UTF8

d87f031

nathanl reviewed Aug 24, 2016
View reviewed changes

nathanl mentioned this pull request Aug 24, 2016

File.write doesn't take advantage ofwritev #5150

Closed

josevalim reviewed Aug 24, 2016
View reviewed changes

Small tweaks from PR feedback

d20f3ba

lexmag reviewed Aug 24, 2016
View reviewed changes

Style tweak based on PR feedback

332a6ab

fishcakez reviewed Aug 24, 2016
View reviewed changes

Only use raw mode if requested.

9d73c97

Also, when not using raw mode, convert data to binary.

Copy close handling of :file.write_file/3

2062372

See https://github.com/erlang/otp/blob/5492ce9951aced8686dbef99d0693e7c6da50c7d/lib/kernel/src/file.erl#L400-L419

josevalim closed this Aug 26, 2016

nathanl mentioned this pull request Aug 26, 2016

Ensure that file:write_file/3 can use writev if given raw mode erlang/otp#1149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let `File.write` and `File.write!` take advantage of `writev` #5154

Let `File.write` and `File.write!` take advantage of `writev` #5154

nathanl commented Aug 24, 2016 •

edited

Loading

nathanl Aug 24, 2016

josevalim Aug 24, 2016

josevalim Aug 24, 2016

nathanl Aug 24, 2016

lexmag Aug 24, 2016 •

edited

Loading

lexmag Aug 24, 2016

nathanl Aug 24, 2016

nathanl Aug 24, 2016 •

edited

Loading

whatyouhide Aug 24, 2016

nathanl Aug 24, 2016

fishcakez commented Aug 24, 2016

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016 •

edited

Loading

fishcakez commented Aug 24, 2016

josevalim commented Aug 24, 2016 •

edited

Loading

nathanl commented Aug 24, 2016 •

edited

Loading

fishcakez commented Aug 24, 2016

josevalim commented Aug 24, 2016

fishcakez commented Aug 24, 2016

nathanl commented Aug 24, 2016

fishcakez Aug 24, 2016

nathanl Aug 24, 2016

josevalim commented Aug 24, 2016

nathanl commented Aug 24, 2016 •

edited

Loading

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016

josevalim commented Aug 26, 2016

nathanl commented Aug 26, 2016

josevalim commented Aug 27, 2016

nathanl commented Sep 29, 2016

josevalim commented Sep 29, 2016

fishcakez commented Sep 29, 2016

Let File.write and File.write! take advantage of writev #5154

Let File.write and File.write! take advantage of writev #5154

Conversation

nathanl commented Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexmag Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanl Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcakez commented Aug 24, 2016

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016 • edited Loading

fishcakez commented Aug 24, 2016

josevalim commented Aug 24, 2016 • edited Loading

nathanl commented Aug 24, 2016 • edited Loading

fishcakez commented Aug 24, 2016

josevalim commented Aug 24, 2016

fishcakez commented Aug 24, 2016

nathanl commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Aug 24, 2016

nathanl commented Aug 24, 2016 • edited Loading

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016

nathanl commented Aug 24, 2016

josevalim commented Aug 24, 2016

josevalim commented Aug 26, 2016

nathanl commented Aug 26, 2016

josevalim commented Aug 27, 2016

nathanl commented Sep 29, 2016

josevalim commented Sep 29, 2016

fishcakez commented Sep 29, 2016

Let `File.write` and `File.write!` take advantage of `writev` #5154

Let `File.write` and `File.write!` take advantage of `writev` #5154

nathanl commented Aug 24, 2016 •

edited

Loading

lexmag Aug 24, 2016 •

edited

Loading

nathanl Aug 24, 2016 •

edited

Loading

josevalim commented Aug 24, 2016 •

edited

Loading

josevalim commented Aug 24, 2016 •

edited

Loading

nathanl commented Aug 24, 2016 •

edited

Loading

nathanl commented Aug 24, 2016 •

edited

Loading