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

Allocation free printing: remove write(a: varargs[string]) #197

Closed
krux02 opened this issue Feb 27, 2020 · 13 comments
Closed

Allocation free printing: remove write(a: varargs[string]) #197

krux02 opened this issue Feb 27, 2020 · 13 comments
Labels

Comments

@krux02
Copy link
Contributor

krux02 commented Feb 27, 2020

Allocation is expensive. The costs are in allocation, deallocation, memory fragmentation (mostly a problem for long running server applications) and in garbage collector activity. Therefor I have a big interest to keep my main loop and especially tight loops free of allocations. Yet almost all string conversion and printing functions rely on temporary intermediate string objects (allocation). What I would like to have is a flexible printing mechanism, that does not allocate by construction.

There is a family of procs already existent in Nim that is almost already what I would like to have. The family of proc write*(f: File, r: float32) in io.nim. It works on overloads for different types, therefore it is extendable for custom types (unlike fprintf in C), it is implemented usually though with fprintf from C, which is completely allocation free. There is one big problem though: proc write*(f: File, a: varargs[string, `$`]) exists. This means expressions such as stdout.write(x) that would normally have no allocation at all, now might resolve to the varargs[string, `$`] overload without providing feedback to the programmer that this has happened. Variables of a custom types that do have an overload proc write*(f: File; a: CustomType) might still secretly resolve to the varargs[string, `$`] overload, because nim-lang/Nim#11225 has not been resolved yet.

Another big advantage of write over echo is, when it crashes when half of the line is printed, that half of the line is actually sent to stdout/stderr. Unlike echo where nothing is printed at all.

So my proposal is to remove proc write*(f: File, a: varargs[string, `$`]) from io.nim to have a style of writing to files and stdout that is free of allocation by construction.

Here is the PR for this RFC: nim-lang/Nim#13277

There is a related RFC targeting fewer allocations for string conversations:
#191

  • This RFC aims for 0 allocations for any printing command, where the RFC above aims for 1 allocation per printing command.
  • This RFC targets only printing to files, stdout and stdin. It does not solve priting to strings.
  • This RFC barely changes anything. And what it changes is currently not used in a lot of code (this means it should not cause any problems).
  • I don't know browser.
@Varriount
Copy link

Hrm, that PR creates quite a significant breaking change. Besides, what about similar procedures, like echo? How could those be made allocation-free?

@krux02
Copy link
Contributor Author

krux02 commented Feb 29, 2020

echo could be made allocation free by forwarding to write. But that is quite a breaking change because it would not call the $ operator anymore. The same argument applies to this fix of write as well, the only argument why it is ok to apply this break here is, write is used much less.

In my opinion there is no way around allocation free printing other than evading the $ operator and finding a different solution entirely. This is my proposal. Wit this you have allocation and $ based echo and allocation and write-overload write and writeLn. I would then simply transition from using echo to write.

@cooldome
Copy link
Member

Allocation is expensive compared to string conversion, but it is cheap compared to writing a file. Writing to a file is done is buffered way anyway. You allocate a buffer, fill in the buffer and you flush the buffer when it is full. This RFC needs at least some evidence that performance will be greatly improved by making this breaking change.

@krux02
Copy link
Contributor Author

krux02 commented Mar 1, 2020

Well, writing to a File is not necessarily "writing to a file". A File can be file, but it can also be piped into another program. Usually a File is buffered. This buffer is a ring buffer that blocks when it is full and you want to write to it, or it blocks when it is empty and you want to read from it. Performance characteristics of such a ring buffer are known. If the buffer is full, and the buffer needs to be flushed to a real file on the disc, then the File is actually slow to use. But the slowest way to use stdout is when printing to a terminal since font rendering (usually done in software) is orders of magnitude slower than writing to a file.

But even if allocation would always be orders of magnitudes faster than writing to any File object. Allocation still might cause memory fragmentation. And there are use cases where memory fragmentation is just not acceptable. And the costs of memory fragmentation are very hard to put in a unit test.

@Araq
Copy link
Member

Araq commented Mar 3, 2020

I like the proposal but your PR does much more than just removing the hurtful write overload. Why not simply remove it and learn from past mistakes and introduce the new better way of doing things in a new module that then can use macros and templates as required.

The motivation is also a bit poor, fragmentation depends on the allocator and is only a problem once it was detected to be a problem (rarely!).

But a printf that is low level enough to be useful inside Nim's GC or on embedded devices that hardly have any memory at all etc would be a welcome improvement.

@krux02
Copy link
Contributor Author

krux02 commented Mar 3, 2020

I like the proposal but your PR does much more than just removing the hurtful write overload.

Well when I remove the hurtful write overload, packages and tests will fail to compile. I need to provide substitutions. And that is exactly what I did here, nothing more.

Why not simply remove it and learn from past mistakes and introduce the new better way of doing things in a new module that then can use macros and templates as required.

Because that would break real existing code.

The motivation is also a bit poor, fragmentation depends on the allocator and is only a problem once it was detected to be a problem (rarely!).

Well what I had in mind when I was writing that was this talk: https://youtu.be/1faaOrtHJ-A?t=1950
This is generally a very interesting talk to watch from beginning to end. But in the time step given, he mentions memory fragmentation being a very big problem when he wrote the file server for GuildWars that was designed to run for years. And yes fragmentation depends on the allocator, that is also what he says, but no allocation has no fragmentation no matter what allocator is in use.

But a printf that is low level enough to be useful inside Nim's GC or on embedded devices that hardly have any memory at all etc would be a welcome improvement.

Well that is another argument I will put in in the description.

@Araq
Copy link
Member

Araq commented Mar 3, 2020

Because that would break real existing code.

But your RFC is about removing this one write overload that is harmful, so it's about a breaking change. Which is fine but we can handle the code breakage differently, say via a --define. Incidentically the whole io.nim file should not be part of system.nim, it should become an explicit import.

@krux02
Copy link
Contributor Author

krux02 commented Mar 3, 2020

But your RFC is about removing this one write overload that is harmful, so it's about a breaking change.

My PR only breaks code that hypothetically somebody could have written. That is a difference to "it breaks nimble packages".

Which is fine but we can handle the code breakage differently, say via a --define. Incidentically the whole io.nim file should not be part of system.nim, it should become an explicit import.

Didn't you just say to not mix refactorings with bugfixes? Anyway, I am not against moving io.nim into it's own real module and therefore thin out the system module. But that is not part of this RFC and I don't think it is tied to this RFC.

But regarding being able to use macros. This is the standard library, we don't need to use macros. If macros are not available we can always move things behind a magic proc instead.

import macros

macro distributeTemplateVarargs(call: untyped, args: varargs[untyped]): untyped =
  call.expectKind nnkCallKinds
  result = newStmtList()
  for arg in args:
    let call = copyNimTree(call)
    call.add arg
    result.add call

template forward(args: varargs[untyped]) =
  distributeTemplateVarargs(echo(" --> "), args)

# test it
forward(1,2,3, "foobar", "abc")

# output:
# --> 1
# --> 2
# --> 3
# --> foobar
# --> abc

I would probably introduce a magic proc that does the same as the macro distributeTemplateVarargs as I often thought that such a thing would be very useful in system.nim.

@Araq
Copy link
Member

Araq commented Mar 3, 2020

Didn't you just say to not mix refactorings with bugfixes?

Yeah. And it's got nothing to do with an RFC process and the begin of an implementation.

Anyway, I am not against moving io.nim into it's own real module and therefore thin out the system module. But that is not part of this RFC and I don't think it is tied to this RFC.

For me it is though because I've seen your PR and it's currently full of bad code like emulating a varargs mechanism.

@zah
Copy link
Member

zah commented Mar 6, 2020

There is a way to implement almost allocation-free printing, but it requires a more significant departure from the current design. It's the mechanism I'm using in Chronicles.

Each echo statement creates a lightweight output stream (admittedly, this needs to allocate memory, but we need only a single output buffer for the entire statement and we can use a specialized allocator that reuses an existing thread-local buffer most of the time).

Each parameter of the echo statement gets forwarded to a type-specific writeToStream function. These functions will usually produce their output byte by byte and this will simply advance a pointer stored in the stream (this is the implementation in nim-faststreams).

Finally, after all the parameters are written, the entire buffer of the stream is sent to stdout.

Chronicles will support this model of printing with any format supported by our serialization library. The human-readable text representation produced by $ can be considered a format called simply Text.

@krux02
Copy link
Contributor Author

krux02 commented Mar 7, 2020

@zah What you are describing is technically the same or at least very similar to what I implemented in the PR. But instead of the new writeToStream function, I use the already existing write function in io.nim (included from system.nim. And to make sure that the write function does not allocate on its own, I have to remove the overload that does allocate for each argument. And technically speaking, writing to a File object writes to a preallocated buffer. The only difference is the preallocated buffer in File lives in the kernel space, not in the process space (for the Posix world at least).

@Araq
Copy link
Member

Araq commented Apr 18, 2020

Here is my plan, and I think it actually aligns well with yours. We remove the io part of the existing system.nim and make people write an explicit import io statement. That's the better design for JS and embedded targets anyway. This new io module is quite like the existing stuff that is currently part of system but it does lack the overload you seeked to remove. Then all that is left to do is to offer a switch like --legacy:io for a migration period.

@github-actions
Copy link

This RFC is stale because it has been open for 1095 days with no activity. Contribute a fix or comment on the issue, or it will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 25, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants