writeFileSync of a large file on Windows seems to exhibit poor behavior.

[CyrusNajmabadi]

In our project we often write multi-megabyte strings out to disk UTF8 encoded. We're finding that writeFileSync seems to perform quite poorly here (on the order of a mb/s). Profiling this showed that the creation of the buffer object to store the encoded data was the culprit.

We were able to work around the issue by breaking the string into 4k chunks and manually creating buffer objects for them that we then wrote out. This reduced write time for a large file from 2 seconds to 80ms.

Interestingly, even with the change, the majority of the time (72 of 80ms) is still spent in buffer creation.

It seems unfortunate that writeFileSync should exhibit such poor performance because of its dependence on Buffer's scaling issues. Technically there is no bug here wrt correctness. However, i feel that the performance issue is extreme enough to warrant a fix at the framework level so that it does not affect others.

Thanks!

[othiym23]

The most idiomatic Node way to write large volumes of data is to use streams to write the data out as they come in. Is there a reason you can't use streams?

The fs.*Sync methods are generally considered convenience functions and not meant to be used in performance-critical code. Writing that much data at once is likely to cause your event loop to block and degrade the performance of your entire app.

[CyrusNajmabadi]

Why would writing that much data out at once degrade the performance of the entire app? Thanks!

As you can see from my note above, writing out all this data at once with the chunking method is only 80ms. That's far less time than we spend on nearly anything else. The performance is heavily impacted though due to the buffer class not behaving well with large strings.

Note: this issue exists even without writing. writeFileSync is not the hotspot. The Buffer class is the hotspot that impacts anyone that uses it.

"The most idiomatic Node way to write large volumes of data is to use streams to write the data out as they come in. Is there a reason you can't use streams?"

If streaming data is fast, then should writeFileSync stream the data :)

[othiym23]

Why would writing that much data out at once degrade the performance of the entire app? Thanks!

Because writeFileSync is a blocking operation, and it writes the entire file before the event loop turns over to the next tick.

Note: this issue exists even without writing. writeFileSync is not the hotspot. The Buffer class is the hotspot that impacts anyone that uses it.

For all I know, copying the string to the Buffer is an O(n) operation, but it still has to allocate a huge chunk of memory, and then that memory has to be dealt with by the GC. Eating that whole cost up-front by copying the whole string into a Buffer is definitely not optimal.

If streaming data is fast, then should writeFileSync stream the data

That's all up to you! If the data is coming over the network, or from another file, or from another stream, it should be as simple as inStream.pipe(outStream).

[trevnorris]

@CyrusNajmabadi large Buffer creation is slow because every time external memory is allocated you need to AdjustAmountOfExternalAllocatedMemory. When the value is large the v8 will spend a lot more time in garbage collection attempting to cleanup any objects which have data. This is normal v8 behavior.

One way you can get around this is to create one large Buffer and reuse it for each write. Basically something like the following:

// 1GB is the max, so we'll use that for demonstration
var large_buffer = new Buffer(0x3fffffff);

while (data_exists === true) {
  var byteLength = large_buffer.write(large_string_data);
  fs.writeFileSync('some_file.txt', large_buffer.slice(0, byteLength));
}

This way v8 doesn't need to spend time constantly trying to cleanup old buffers, but in the end this is not an issue with node, but with v8.

[tjfontaine]

@trevnorris has been putting significant effort in #4964 to remedy some of this. You should really be looking at making fewer buffers (and smaller) where ever you can.

But lets talk about the fundamental misunderstanding you're having here, which @othiym23 has been mentioning. If you call something that's named "sync" nothing else can happen in node, and any such calls should be the exception, not the rule.

This multi megabyte buffer you have was not synthesized out of thin air, more than likely it's filled with data from an external source, a file, a db, a network stream. As you're consuming that incoming data you should be turning around and writing it out to where you need it.

There are many interesting facilities that expose that pipe interface that @othiym23 mentioned, you can find them at http://nodejs.org/api/stream.html but perhaps what you're looking for is to implement a Transform stream.

[CyrusNajmabadi]

"Because writeFileSync is a blocking operation, and it writes the entire file before the event loop turns over to the next tick."

That is not a problem for our app scenario. We're happy to block on writing the file. As I mentioned above. This is only actually about 8ms. The problem the layers above where writeFileSync and Buffer don't interact well with eachother. writeFileSync decides to allocate an entire buffer up front, even though Buffer doesn't scale well with that. This pushes the problem onto all consumers of writeFileSync (and possibly even writeFile, though I haven't checked it) to ensure that they don't pass too large a buffer in.

[CyrusNajmabadi]

"For all I know, copying the string to the Buffer is an O(n) operation, but it still has to allocate a huge chunk of memory, and then that memory has to be dealt with by the GC. Eating that whole cost up-front by copying the whole string into a Buffer is definitely not optimal."

Agreed. writeFileSync should be much smarter here if Buffer is so exacerbated by large contents.

Note: the behavior definitely doesn't seem O(n) (though with constants who can ever be sure). After all, 1000 8k buffer chunks was orders of magnitude faster than 1 8MB buffer.

If it was O(n) we expect comparable time for each. But we're seeing a huge discrepancy with a large buffer.

[CyrusNajmabadi]

"That's all up to you! If the data is coming over the network, or from another file, or from another stream, it should be as simple as inStream.pipe(outStream)."

I'm not doing any of that though. I simply have a large string that i'd like to write. The API purports to be able to do that. However, it is extremely slow due to its mishandling (or misunderstanding) of the perf requirements of the Buffer class.

As mentioned already, this issue has been worked around by switching to manually creating buffers ourselves and working around the scalability constraint contained within. However, this does not address the case where Node libraries (like FS) artificially expose consumers to those same scalability concerns.

[CyrusNajmabadi]

"@CyrusNajmabadi large Buffer creation is slow because every time external memory is allocated you need to AdjustAmountOfExternalAllocatedMemory. When the value is large the v8 will spend a lot more time in garbage collection attempting to cleanup any objects which have data. This is normal v8 behavior.

One way you can get around this is to create one large Buffer and reuse it for each write. Basically something like the following:"

I don't think you're understanding. In the bug that is being reported we don't create a buffer at all. We have a string. The problem is that when we call writeFileSync writeFileSync decides to create a buffer, not realizing everything you just wrote. As writeFileSync is a library call written to sit on top of V8, it should be aware of the performance constraints of that underlying system and should do the necessary work to not exacerbate them.

Note: in our workaround we address this by doing precisely what you propose. i.e. we accept that writeFileSync improperly uses buffers, and we do the buffer management ourselves. However this adds complexity and does not help anyone else who ends up needing to call writeFileSync. An appropriate fix would be either at the fs level so that consumers of fs don't need to work around underlying implementation details.

[CyrusNajmabadi]

"But lets talk about the fundamental misunderstanding you're having here, which @othiym23 has been mentioning. If you call something that's named "sync" nothing else can happen in node, and any such calls should be the exception, not the rule."

Again, that is fine for our use case. We have literally nothing else to do in this scenario. We are simply taking a large string and writing it to disk. That is the scenario. And, again, to be clear, writing the file is not the problem. Not at all. Writing the file is extremely fast and absolutely not a bottleneck.

The issue entirely (to the tune of 99.9% of the scenario) is the cost of Buffer's constructor. And that is out of our control because it is fs.writeFileSync that decided to blindly create a buffer with the string contents that were provided to it.

Now, I absolutely get where you're coming from. There certainly appear to be APIs provides that workaround the issue. However, this bug was not filed to find a workaround. We already have a workaround. We've already addressed this by avoiding the pit-of-fail exposed through this API.

The purpose of this bug was to get the library function, which is exposed to all node consumers, to be written in a smarter fashion so that this will not be a problem for them.

For example, writeFileSync today is written as (In psuedocode):

openFileSync
createBufferFromString
writebufferToFileDescriptorSync
closeFileSync

This implementation is entirely under your control, and it can be written in a smarter fashion to handle large strings, so that consumers don't face a 100x hit for the choice to create buffer out of that entire string.

For example, all the workarounds you mentioned like creating a single small buffer, filling it, and writing that out, can be inlined into that code.

This fixes the issue for all consumers and makes writeFileSync have great perf.

As this is a library function, it should get the same care and attention to its performance as anything else. :)

[CyrusNajmabadi]

"This way v8 doesn't need to spend time constantly trying to cleanup old buffers, but in the end this is not an issue with node, but with v8."

This is incorrect AFAICT. But maybe you can educate me on this. Who owns the implementation code for fs.writeFileSync? Is that node or v8? If it is 'node', then this is an issue with node. Because node has chosen to call certain v8 functions blindly, rather than more intelligently handling things. if fs.writeFileSync is indeed v8, then this would be a v8 bug to fix.

[othiym23]

Node is optimized for nonblocking I/O. The fs.*Sync operations are provided for limited convenience use. The vast majority of the core team's attention is on making streaming and nonblocking operations fast. Fixing this seems like something that would be very low on the team's list of priorities. I'm sure a well-tested pull request that addresses this (and doesn't have any regressions) wouldn't go amiss, though.

[CyrusNajmabadi]

That's fine. I'm absolutely fine with this being a low priority bug. I just was trying to avoid what seemed to be people indicating that this was by design. As long as there is an appropriate work item tracking this and documenting the currently poor behavior surrounding writeFileSync with large files then i'm happy.

I'm def willing to try out a pull request for you guys.

Thanks!

[tjfontaine]

Please provide a test case, sample data, performance numbers, and information about your platform while interested parties investigate, then we can put the "why you're doing it wrong" discussion to bed.

[CyrusNajmabadi]

Will do. Is there a preferred test case format you want? Or will a simple node app be acceptable?